Add missing claro/classy templates to Admin theme

This came about because gin has base theme: claro and default_admin has base theme: false. We might need to double check whether there are other claro templates, etc that need to be copied into default_admin as well.

I think that's best answered by an FEFM or the admin theme maintainers.

In what backend situation does this theme need these templates?

#9 won't this be the only theme shipped with core moving forward? So it's technically not just backend right?

The README.txt inside the classy templates folder:

"WHAT IS THIS DIRECTORY FOR?
--------------------------------
This directory is for templates previously inherited from the Classy theme.

WHY WERE CLASSY TEMPLATES COPIED HERE?
-------------------------------------------
Classy was removed in Drupal 10. To prepare for Classy's removal, templates that
would otherwise be inherited from Classy are copied here."

So, I assume the same procedure needs to be followed 'To prepare for...' Claro's removal?

Tried removing the classy directory from claro templates. Not getting any WSOD. Themes seem working okay. I assume there was a preprocess? function in Claro theme during removal of classy that wired up the classy directory. Once the removal of classy was complete, that function was removed. So possible clean up of Claro by removing Classy folder as first task eg create a child issue?

Not getting any WSOD

Why would you get a WSOD?

I assume there was a preprocess? function in Claro theme during removal of classy that wired up the classy directory

Also confused by this, what would a preprocess function do?

I think this issue is more what to do about the field templates in default_admin than in claro.

edit: Re: #14 On closer inspection claro/templates/classy/field contains 13 templates. claro/templates/field and default_admin/templates/field only contain 1. So copy those 13 templates into some directory in default_admin theme.

A missing template will not WSOD it will just use a less specific template.
Also the loading of template overrides is core functionality as long as it's in a theme it will load, there is no custom code for that.

The classy folder still works in claro.

This issue is about whether we need similar templates in default admin.

I am not up with latest theming. But to create a new subfolder inside a theme's templates and get the templates inside it to override depends on the name of the templates. But for the subfolder to be 'recognised' by the theme is there a function needs to be created? Not sure there is anything inside the templates that will need editing.

The current steps to reproduce are not reproducing the issue. The issue is that certain tests are failing.

Set default_theme to default theme. Ran drush command to create article content type. Created a test article with body field. Created an article with the word 'test' in the body field. Currently the field markup is:

<div><p>test</p></div>

After I copied the field templates from classy subdirectory of claro theme into default_theme/templates/field directory:

<div class="clearfix text-formatted field field--name-body field--type-text-long field--label-hidden field__item"><p>test</p></div>

Re: #19 Maybe you should try #20 and see if the tests work?

Re: #19 I am not sure anyone really knows that the issue is let alone what needs doing on it.

After I copied the field templates from classy subdirectory of claro theme into default_theme/templates/field directory:

Exactly, that is what this issue is trying to decide if we need to do.

Or if the lack of classes is acceptable.

Re: #24:

It's very much appreciated

Right.

Re: #23:

Exactly, that is what this issue is trying to decide if we need to do.

and #24

I am setting this to Postponed to prevent more 'noise' and pending framework manager/ subsystem manager review.

This is at Needs work. What work needs doing? If there is, please flip back to Needs work and specify what work needs doing from this point on.

I am a certified Symfony 6 developer. The exam covers Twig templating. I have professional experience of theming Drupal 8+.

While no work is needed on the MR, this still needs discussion.

Adding a summary of a slack discussion, https://drupal.slack.com/archives/C079NQPQUEN/p1789139357811479,
with smustgrave, jergenhaas, godotislate, joachim, catch, Steven Jones, Gábor Hojtsy, KentR and mstrelan.

Converting some tests from Claro to Default Admin hit a snag where divs in Default Admin do not have field classes but Claro does. jurgenhaas thought this was a bug, and the solution seemed to be to copy over the necessary templates. Then it was questioned if a back end theme needs those templates at all. There wasn't disagreement on that point. godotislate suggested changing the tests to search for the field content in the page, an idea catch agreed with.

So, for now this is a won't fix.

Re: #29

There wasn't disagreement on that point. godotislate suggested changing the tests to search for the field content in the page, an idea catch agreed with.

I followed the steps to reproduce on a vanilla Drupal 12 site and 'the field content' is present and visible. The attributes inside the div tags are missing.

So will the attributes be needed if 'godotislate suggested changing the tests' happens?

If not then we should change this issue's (current) status to Closed (won't fix).

If the attributes are going to be needed then we need an IS update to explain why.

My opinion is that I don't think there's no harm adding the template. If people used it in Claro, they'll want the same functionality in Admin. No hard reason not to put them in 🤷‍♂️

If we decide to put this in, I'd like to run a visual regression test using https://github.com/mherchel/ddev-drupal-admin-vrt

Re: #35 If so, explain how and update the IS. #29 shows that catch and godotislate agreed that the content of the fields is all that is needed for the tests to work because the tests will be re-written to depend only on the content of the fields, not the div attributes which is what this issue is all about.

My opinion is that I don't think there's no harm adding the template. If people used it in Claro, they'll want the same functionality in Admin. No hard reason not to put them in 🤷‍♂️

That sound like a feature request, not a bug.

Re: #4

We might need to double check whether there are other claro templates, etc that need to be copied into default_admin as well.

That is a lot more work. Do we need a follow-up or within scope?

New title something like: Add missing templates to default admin theme and adjust CSS styling

My take is that we need to bring Classy templates back, not just the field template.

Our original intention, when we moved Gin into Core with #3556948: Merging Gin as Admin theme, was to merge all inherited templates into Gin's template directory to maintain the same behavior as before, The plan had been to clean-up the templates, including their directories, names, attributes, etc. in a follow-up step, but we didn't fully address that yet.

Then, #3582018: Remove or move the uses of classy templates in default admin theme happened, and I didn't pay attention back then. As part of that clean-up effort, all those intentionally included templates got removed, except for three of them. All others have been removed.

My take is, we should bring them all back to maintain the same behavior, unless default_admin covers a removed template already. 43 templates got removed, 3 (block, page, and region) got moved to their correct places.

I think we should take the effort and analyze those 43 and decide individually, if and how much of them needs to be migrated.

Re: #42 jurgenhaas, If you check the current MR you will see that not only have the classy field templates been copied but css/ pcss files also. And then those files have been edited. Were those css-related tasks your 'original intention'? That could potentially involve a lot of additional work. So is this going to delay the removal of Claro? Do we need to do the work you mention in #42 only before removal of Claro? Or do we also need to also copy and edit all related css/pcss files?

Thank you @smustgrave, great job.

@oily re #43, yes, the css recovery is part of this template recovery. And @smustgrave has covered that nicely. And no, I don't see any reason for a Claro removal delay. The work on default_admin ensures, that it has no dependencies on neither Claro nor anything else from the former chain. And while we now may have some redundant components in default_admin, we're not missing anything from the existing functionality, with or without Claro. So, subsequent clean-ups can be performed on default_admin, but Claro can be removed, at least with regard to this issue's scope.

Re: #46 Danke, jurgenhaas.

The current IS states:

Marked critical because this probably blocks claro's removal

Downgrading to edit: Major.

Re: #42 I see a downside to copying the same templates of these old admin themes. It seems the goal is to reproduce the same 'look' also by preserving the same layout of elements bolding etc and also assets like .svg's.

But if that is our goal, users of the default admin theme will get a strong sense of deja vu. They will instinctively feel they are using classy or gin or claro. The UX effect does not therefore seem good. Pouring new wine into old bottles, so to speak.

Should we not swap out at least the more prominent .svg's with new ones. Sort of like creating a few landmarks that tell the user they are in a brand new admin theme? Or do we have such 'landmarks' in place already?

@oily the objective has been to bring Gin into core. So, if the result looks like Gin, there is nothing wrong with that. However, we have a lot of folks working on significant improvements for that foundation. And what we'll get will be the best admin theme ever. But that work would be disrupted if foundational components were just missing.

Re: #52 @jurgenhaas Ah, interesting.

However, we have a lot of folks working on significant improvements for that foundation.

So the CSS edits in the MR for this issue are/ should be to create a sound baseline? To exclude browser errors only? The 'visual regression test' tool mentioned at #32 is that a good way to evaluate the code in this issue? Else how does a reviewer decide that the CSS is good and ready for RTBTC?

i think bringing these templates back makes sense, especially if they were already being used through Classy/Claro before.

Just wondering, after this is merged, do we plan to go through these templates and CSS one by one and remove the ones that Default Admin doesn't really need? Or would that be better as a separate issue?

@oily, re. #53 and #54 this issue is to restore what was lost a few months ago. And in separate issues, the UX and CSS cleanup is under way already. That's not within scope for this issue.

@nitinkumar_7, I'd like to leave that to @mherchel who is leading that effort.

I reviewed the MR by comparing the templates being added to those that were removed by #3582018: Remove or move the uses of classy templates in default admin theme. I feel that the issue summary needs a little clarification that we aren't simply reverting the changes from #3582018. It appears that we're copying those removed templates from Claro, as well as any attached libraries. This is fine and appropriate because those templates have been updated over time with minor changes by projects like OOP preprocess hooks. But the IS made it sound like this is more of a reversion. I've updated the Proposed Resolution to reflect what's actually being done.

Otherwise I couldn't find anything to comment about. The changes shown in the diffs were minor, mostly caused by routine updates. There were no major changes to the HTML structures.

This is still pending FFM review, so I can't RTBC it, but it gets an RTBC+1 from me.

It was suggested that maybe I could RTBC it while an FFM is pinged for a final check. So here we go.

There's a lot in here. I tested everything (I think), especially media library.

There's some styling bugs on the managed file UI, and I'm about to open up a separate issue that I noticed.

Here is a before pic -
This is what i did:

• Set default admin as the admin theme
• Deleted claro from the core themes
• Cleared the cache

After checking this PR out, I cleared the cache.

This PR works as expected and removes the need for claro.

Some questions on the MR - the logical property might be blocking per @mherchel comment above

https://git.drupalcode.org/project/drupal/-/merge_requests/17092#note_28...

Yeah, a lot of this is old CSS copied over from Classy or wherever. Should be easy enough to fix.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Looks good. I looked at the code changes and they seem to take into account most concerns.
Plus, I downloaded the new commits and did a quick visual pass and switched the window to RTL.

There's one minor valid that @larowlan flagged. Instead of using background-image on every selector, you can use the base selector (which should be .file)

Ok I reviewed this pretty closely, I think all feedback has been addressed. I then went through each template.

The three search ones were not moved, but that seems correct.
media-embed-error.html.twig also was not moved, is that intentional?

Everything else exists in both so I think we're good.

I did see some rss templates, and toolbar, but removing those are likely out of scope here.

I think this is good once we answer about the error.

I did wonder why the file css was the only library that we had to add, the others already exist, but that's not a mystery we really need to solve.

I can't answer @nicxvan questions, but did my own review and found two things: node.html.twig was attaching a missing library. Removed. file.pcss.css and file.css was missing RTL styling. Added.

I did wonder why the file css was the only library that we had to add, the others already exist, but that's not a mystery we really need to solve.

It's entirely possible there were libraries that existed, but never got called. There's still a whole lotta of cruft that came over from Seven>Claro>Gin that needs to be cleaned, but doing so isn't a stable blocker.

We have an issue to clean up the libraries file.

Turns out skipping media-embed-error.html.twig was intentional: https://git.drupalcode.org/project/drupal/-/merge_requests/17092#note_28...

@gabor's change looks good too!

Committed and pushed 04225dd3315 to main. Thanks!