Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix presets in themes that use the default color & gradient palettes #22526

Merged
merged 1 commit into from
May 28, 2020
Merged

Fix presets in themes that use the default color & gradient palettes #22526

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-library/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
// to assure colors take effect over another base class color, mainly to let
// the colors override the added specificity by link states such as :hover.

:root .editor-styles-wrapper,
:root {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just remove root?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that theme and core have the same specificity (020), hence the last declaration wins: changing classes won't solve the issue.

By doing this the specificity in the front will still be 020 (root pseudoclass + preset class) while in the editor is 030 (root + editor wrapper class + preset class). My understanding from reading past convos was that we used :root to cover focus and hover statuses for colors, so we still need something like that. I could perhaps look into change :root by those more specific classes, however, root seems to be working fine for that purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought .editor-styles-wrapper .color was enough since the other conflicting rules are more .editor-styles-wrapper button but I think your approach is safer but why not apply it to font-sizes. I guess we could have the same issue there too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just double-checked, and TwentyTwenty uses the button class, not the element (.editor-styles-wrapper .wp-block-button__link). Probably other themes do this differently, though.

Adding a new selector with the editor wrapper is the same thing I did for font-sizes a few PRs back, so doing this for colors as well makes everything coherent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new selector with the editor wrapper is the same thing I did for font-sizes a few PRs back, so doing this for colors as well makes everything coherent.

Should we add :root there too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 what would be the advantage? I believe for colors it was only introduced to cover hover/focus statuses, so perhaps it makes more sense to change :root to :hover & :focus (although not sure if there are browsers issues with them).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reasons maybe? Don't you think we need to protect these for fonts too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is that I haven't seen reports of that being an issue. I guess color is something people change for those hover/focus statutes (so they enqueue styles) but fonts don't, hence there's no conflict. However, I'm always happy to explore and see what happens. I can prepare a separate PR to gather feedback and address that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a PR to discuss this separately at #22671

// Background colors.

Expand Down