Cover block: Only use white text when the background of the cover block is dark. #33541
Conversation
scruffian
added
the
[Block] Cover
|
Size Change: +107 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
| function useCoverIsDark( | ||
| url, | ||
| dimRatio = 50, | ||
| overlayColor = '#000000', |
There was a problem hiding this comment.
What makes us need to set a default overlay color?
In case we set a default overlay color some code on the hook needs to be updated. For example, the code inside conditions "if ( ! overlayColor ) {" and "if ( ! url && ! overlayColor ) {" is never going to be executed (as we will always have an overlayColor).
There was a problem hiding this comment.
The reason we need it is because by default the block background is set to black by CSS.
|
|
||
| } | ||
|
|
||
| &.is-dark-theme { |
There was a problem hiding this comment.
The class is-dark-theme is only applied on the editor side, not on the frontend (not found by searching in packages/block-library/src/cover/save.js). Styles that apply only on the editor side should be on "packages/block-library/src/cover/editor.scss", to avoid loading styles on the frontend that are not used.
There was a problem hiding this comment.
This has been problematic in the past. It's easy to confuse the class, discovered via inspecting in the editor, and assume it's also on the frontend, when as you suggest it's only about the block UI being contrasty (such as a white focus style for blocks when the theme is dark, vs. a blue focus style). The class has also been confused with being related to operating system dark modes, whether for the theme or for the editor.
We might need to reconsider that UI inversion feature entirely, either automate it so it works without a CSS class, or find some other way to explore contrast in such cases.
| @@ -604,7 +618,7 @@ function CoverEdit( { | |||
| const classes = classnames( | |||
| dimRatioToClass( dimRatio ), | |||
| { | |||
| 'is-dark-theme': isDark, | |||
| 'is-light': ! isDark, | |||
There was a problem hiding this comment.
is-light is preferable, since the current default for the block is dark. We can't update all the blocks that already exist in post content, so the class works better as a modification to a new style rather than a change to the existing rules.
| dimRatio = 50, | ||
| overlayColor = '#000000', | ||
| elementRef, | ||
| setAttributes |
There was a problem hiding this comment.
I'm not sure if passing in setAttributes is the best approach here, but it works :)
| @@ -51,6 +51,10 @@ | |||
| }, | |||
| "contentPosition": { | |||
| "type": "string" | |||
| }, | |||
| "isDark": { | |||
There was a problem hiding this comment.
Do we need to updates the deprecations when adding an attribute?
There was a problem hiding this comment.
From my tests, we don't...
|
One issue I see with this, which I am not sure how serious it is: When I open a page with a cover block in which has a light background, the 'is-light' class is applied and therefore the text changes from white to black. This will alter some users sites in an unexpected way. They can still set the color of the text back to white if they want to, but it might be a problem. What do you think? |
scruffian
force-pushed
the
update/cover-block-colors
branch
from
1dfae20 to
fe659dc
Compare
glendaviesnz
force-pushed
the
update/cover-block-colors
branch
from
fe659dc to
3c46f07
Compare
|
@scruffian, I rebased and put the is-dark-theme selector back as discussed - it all seems to be working as expected on TT1-blocks, seedlet, twentytwenty and twenty-nineteen, but not on TT1 - on that theme the text doesn't change to white when the background is dark, haven't managed to work out why yet, but will have a bit more of a look on Monday. |
|
@scruffian I had to add some more specificity to the text colour selectors to get this fix to work on TwentyTwentyOne as it has a With this in place it seems to work as expected with dark and light images and background colours, and the text colours can still be overridden at the innerBlock level:
|
glendaviesnz
force-pushed
the
update/cover-block-colors
branch
from
ba39afa to
c1cfb8f
Compare
Does the default style work well in other themes? The reason it's best to keep specificity as low as possible, even if will require theme changes, is that as global styles eventually expands to enable styling more things, it will only work if blocks and themes use low specificity in the place where it connects. |
It worked as expected in all the other themes I tested on, but another option would be to update TT1 - @scruffian, do you have any thoughts on this as a theme team member? |
|
My preference would be to update TT1 as @jasmussen suggests. |
|
@jasmussen, @scruffian I have reverted the additional selectors that were added for TT1, and this is now working as expected on TT1-blocks, seedlet, twentytwenty and twenty-nineteen, but not on TT1 if you want to give it another test. Should we finalise and merge this and then open a core ticket for tt1 theme? It might be be easier for people to replicate the issue once this fix is in a gutenberg plugin release, and I don't think this change makes it any more broken in TT1. |
Yeah sounds like a good plan 👍 |
…wentyTwentyOne theme
…s with TwentyTwentyOne theme" This reverts commit 68675e2.
glendaviesnz
force-pushed
the
update/cover-block-colors
branch
from
a62916d to
b7c6727
Compare
I have removed the default and it still seems to work as expected - it still ends up with the attribute set every time as there is always a background that is compares against and the theme default of black is picked up even if a background colour or image hasn't been selected. |
Actually, if we don't set a default it causes validation issues with older blocks, if the default is set it looks like we don't need a deprecation, but without a default we do, so have put the default back. |
|
Seems to work: It seems good that there isn't any added specificity, which lets us avoid future problems with the low default specificity that global styles outputs, and similarly the Edit: I also tested the frontend working correctly in the above. |
Description
Presently the text of the Cover block is always white unless the user customizes it. This causes a problem when the background of the block is light.
This PR updates the conditions on which we make the text white - it is now only white when then block has an "is-dark-theme" class, which gets applied when the block has a dark background color or image.
Fixes Automattic/themes#3483 and #17953
One issue to be aware of is that in a dark theme (for example Quadrat), when the cover block is set to a light background, the default text color for the theme will still be used. Perhaps in a dark theme we should default the block to have a white background and dark text, but that is a much larger change, and this PR doesn't make that problem worse so we can dealt with it in the future.
How has this been tested?
In empty theme, add a cover block. Set the background to a light color and confirm you can still see the text.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).