Skip to content
This repository was archived by the owner on Nov 16, 2021. It is now read-only.

Conversation

@balsama
Copy link
Contributor

@balsama balsama commented Oct 30, 2019

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).

@balsama
Copy link
Contributor Author

balsama commented Oct 30, 2019

Tests are failing, but they are in HEAD of 4.x too.

Copy link
Collaborator

@phenaproxima phenaproxima left a 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.)

@phenaproxima phenaproxima requested a review from lauriii December 12, 2019 18:17
Copy link

@lauriii lauriii left a 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.
Screen Shot 2019-12-13 at 11 06 34

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.
Screen Shot 2019-12-13 at 11 07 12
Screen Shot 2019-12-13 at 11 07 03

Entity Embed could be improved too. Maybe we should open a follow-up in the Entity Embed queue for this?
Screen Shot 2019-12-13 at 11 10 43
Screen Shot 2019-12-13 at 11 10 53

@lauriii
Copy link

lauriii commented Dec 13, 2019

Here's an issue to solve some of the ImageWidgetCrop related problems in Claro: https://www.drupal.org/node/3096241

@balsama
Copy link
Contributor Author

balsama commented Dec 13, 2019

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

@phenaproxima
Copy link
Collaborator

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:

  1. We will merge this PR, include the combined patch, into Lightning 8.x-4.x for now.
  2. When we require Lightning Media 4.x, we will allow it to be the thing that patches core.

@phenaproxima
Copy link
Collaborator

Gave this a brief manual test and I think it looks nice! Since this is fairly "low-stakes", in the sense that:

  • a site owner can easily revert to Seven if they want to
  • this only takes effect for new sites
  • it looks really nice most of the time
  • we are documenting the known issues @lauriii discovered
  • this is a high-integrity commitment that we must deliver (i.e., commit) this quarter

I think this is ready.

@phenaproxima
Copy link
Collaborator

phenaproxima commented Dec 16, 2019

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.

@phenaproxima phenaproxima merged commit 201739b into 8.x-4.x Dec 16, 2019
@phenaproxima phenaproxima deleted the claro-default branch December 16, 2019 18:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants