-
Notifications
You must be signed in to change notification settings - Fork 99
Conversation
|
Tests are failing, but they are in HEAD of 4.x too. |
phenaproxima
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should get @lauriii to sign off on this, since he knows what sorts of patches and/or workarounds are needed in 8.8 to fully support Claro. (There are known bugs with things like shortcuts and other detritus.)
lauriii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some manual testing with Entity Embed, Media Library, and Bulk Media Upload. I found some issues but I'm not sure if I can judge whether they are severe enough to justify blocking this issue.
I found a new bug on the vertical tabs. For some reason on the bulk media upload form, the vertical tabs component isn't rendering the tabs as expectedly. This seems like a bug in Claro. The markup is messed up but it seems most of the critical functionality is still there.

I also noticed that the ImageWidgetCrop isn't 100% compatible with Claro. It does work but it looks clearly off. This seems potentially a problem with the module. We could adjust the way we arrange things in Claro but I'm not sure if we could make any meaningful change there without breaking something else.


Entity Embed could be improved too. Maybe we should open a follow-up in the Entity Embed queue for this?


|
Here's an issue to solve some of the ImageWidgetCrop related problems in Claro: https://www.drupal.org/node/3096241 |
|
Thanks. I moved the existing patch over to Lightning Media and added the patch to help with Image Widget Crop. Unfortunately, they conflict with each other. Failing PR here: https://github.com/acquia/lightning_media/pull/106 |
|
I have rolled a combined patch and merged acquia/lightning_media#106 into Lightning Media 8.x-4.x. This was necessary because this all depends on Drupal 8.8, and Lightning Media 3.x allows multiple minor versions of core. So, to reiterate the plan here:
|
|
Gave this a brief manual test and I think it looks nice! Since this is fairly "low-stakes", in the sense that:
I think this is ready. |
|
Ah, one thing that I would like to see changed: this PR deletes a bunch of blocks we provide for Seven. That means that, should a site builder revert from Claro to Seven, a bunch of things are busted and they have to do work to make their admin experience half-decent again. I suggest we keep the Seven blocks, rather than delete them, so that sites which want to keep using Seven don't have to do as much work. I think that it would be more appropriate to remove them when Claro is officially declared stable. |
This PR swaps out the default admin theme for the beta version of the contrib theme Claro. When Lightning updates to 8.8 it can drop the dependency on the contrib theme since it's included.
It also deletes the block config that used to ship in the profile for Seven. Claro provides its own config objects for blocks. The contrib project has them in /config/install while the core version puts them in /config/optional. But that shouldn't make a difference as they are provided by the theme either way (whereas Seven's block configuration lived in the Standard profile and had to be duplicated in Lightning).