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.

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

Try lowering the color classname specificity #15104

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

With recent changes to reduce the specificity of editor styles, it appears we no longer need the class doubling hack. Please test all the custom colors with a theme that does not specify its own color palette, i.e. you can't test with TwentyNineteen. Verify that colors look correct in the editing canvas, in the frontend. Try both button background colors, paragraph background colors, and any other blocks that uses these colors.

Screenshot 2019-04-22 at 13 14 47

Screenshot 2019-04-22 at 13 14 51

Fixes #12986.

With recent changes to reduce the specificity of editor styles, it appears we no longer need the class doubling hack. Please test all the custom colors with a theme that does not specify its own color palette, i.e. you can't test with TwentyNineteen. Verify that colors look correct in the editing canvas, in the frontend. Try both button background colors, paragraph background colors, and any other blocks that uses these colors.
@jasmussen jasmussen added the [Type] Code Quality Issues or PRs that relate to code quality label Apr 22, 2019
@jasmussen jasmussen self-assigned this Apr 22, 2019
@jasmussen jasmussen requested a review from kjellr April 22, 2019 11:19
@jasmussen
Copy link
Contributor Author

CC: @m-e-h: I know you're busy and you don't have to review this. Just thought you'd appreciate it as a followup to the improvement you made yourself.

@kjellr
Copy link
Contributor

kjellr commented Apr 22, 2019

Colors seem to be working fine for me after this change, but I'm a little unclear about the reasoning behind the double-specificity in the first place. Is there a specific use case I should test out?

@m-e-h
Copy link
Member

m-e-h commented Apr 22, 2019

I absolutely appreciate efforts to decrease specificity @jasmussen!

However, "utility" classes like these are one of the cases where specificity can and IMO should be increased.

I'm seeing some issues with this change. Oddly FireFox and Chrome aren't consistent in how they render this for me. Both have issues though.

I'm using a blank Underscores theme:

In Firefox:
buttonsFF

In Chrome things appear to work until the buttons are :hovered or :focused:
buttonsChrome

@jasmussen
Copy link
Contributor Author

Thank you for testing, and thank you for the feedback. Are your screenshots from the editor, or the frontend, though? I'm seeing the hover issue on the frontend, but not in the editor.

I wonder if there's a next step then, other than closing this PR, and even closing #12986 as "working as intended".

  • Is there a fix for keeping the specificity higher, but not as high as duplicating the classes?
  • Is the fix to expand the comments in the code to explain that the purpose of the higher specificity is to ensure that the colors work even on the frontend, where they'll otherwise be overridden by link colors?

Definitely an interesting specificity challenge here.

@m-e-h
Copy link
Member

m-e-h commented Apr 23, 2019

My testing was done strictly on the front-end. I'm pretty sure an inline style attribute is added for the colors in the editor so that becomes not so relevant here.

Is there a fix for keeping the specificity higher, but not as high as duplicating the classes?

Not that I can think of.
.has-luminous-vivid-amber-color:hover is equal to two class selectors.
I'm curious though, what problem would that solve? What's wrong with the current specificity?

Is the fix to expand the comments in the code to explain that the purpose of the higher specificity is to ensure that the colors work even on the frontend, where they'll otherwise be overridden by link colors?

Maybe worth mentioning links and their different states adding specificity. But this is in style.css so "even on the frontend" should be a given.

I'm a little confused on what #12986 is about.
Reckon I'll post a comment over there.

@jasmussen
Copy link
Contributor Author

I'm curious though, what problem would that solve? What's wrong with the current specificity?

Only that the class duplication is quite confusing to users debugging the code. It's clever, very much so, but it's also hard to parse for anyone but the highest echelons of CSS wizards.

I read your comment on the other post, great comment thank you! I would personally think it sufficient to provide elaborate inline code comments to explain the classes are duplicated, and why the specificity needs to be higher.

Closing this one for now, thanks folks!

@jasmussen jasmussen closed this Apr 24, 2019
@youknowriad youknowriad deleted the try/lower-color-specificity branch May 27, 2020 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.has-vivid-cyan-blue-background-color.has-vivid-cyan-blue-background-color
3 participants