-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Cover block: add option to set opacity when background colour used #35065
Conversation
Size Change: -48 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
I know this is WIP, so just jotting down observations so far. Zero opacity (at least according to the range control is darker than 10 or 20 opacity. Something to do with the default background color for At least in TwentyTwentyOne, the value of The latter might be a theme thing though. Will keep testing tomorrow. Thank you! |
For some reason the opacity setting explicitly excluded setting opacity to 0 - not really sure why, so have allowed this now, but need to have a think about whether there are some unexpected gotchas to doing this that may have been the reason for not allowing 0 opacity previously. |
Not sure how we could get around that, or if it is in fact an issue - open to ideas? |
I found something else strange:
Is it because we set the I guess we either need to reset the opacity value to number or keep the state after we clear the media?
Not sure if it's an issue either. I checked Twenty Twenty for example, which doesn't have this problem at all. In relation to the deprecations, I tested out creating a new deprecation But as @aaronrobertshaw pointed out in our discussions, it looks like we might have to do the same for the image/video/gradients etc unless I'm mistaken. |
Nice catch! I have pushed a fix for this, which is to just leave the opacity as is when image is removed - I think the idea of clearing it probably relates to the fact that previously it was irrelevant if there was no media, but now it seems to make more sense to leave it as is - but let me know if you think there is any flaw in my logic here. |
Thanks! This sounds legit to me. My reasoning is that if we go to the trouble of setting an opacity, it seems unexpected to have that removed as a side effect of clearing an image. |
2a15af5
to
2e9386d
Compare
2e9386d
to
5018ded
Compare
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.
nice catch @apeatling - I only tested custom gradients, which work, and not the preset gradients. I have pushed a fix for this if you can retest please. |
ba69914
to
d45216b
Compare
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.
Confirmed this is now working when switching between solid colors and pre-set and custom gradients. 👍
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.
Nice work @glendaviesnz! The deprecations are looking good to me now.
I ran into a couple of issues while testing on some classic themes:
In TwentyTwentyOne, the theme sets a black background color on the .wp-block-cover
class. So on the front end of the site, the opacity fades the color over black instead of the parent cover block with the background image. I imagine this one will need to be fixed in that theme instead?
On the front end of the site running TwentyTwenty and TwentyNineteen, if I choose one of the preset colors, the background color is getting overridden with a rule that's trying to use a CSS variable that doesn't appear to be present. I'm not sure if this is a theme issue or a regression elsewhere in Gutenberg? (The screenshots are a little small, but in inspect element, it says --wp--preset--color--accent
or --wp--preset--color--primary
is not defined). So, possibly not caused by this PR, but it'd be good for us to figure out why it isn't working 🤔
TwentyTwenty | TwentyNineteen |
---|---|
I also ran into an issue with using a custom color in the rendered markup instead of one of the preset colors. Looks like it should be a simple fix, so I've added a comment.
Happy to do more testing, too!
I think we should open an issue for this in the theme repo once this PR is merged. There is a similar issue with text color in this block in TT1 |
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.
Thanks @glendaviesnz, that's testing nicely for me now!
TT1-blocks | TwentyNineteen |
---|---|
I think we should open an issue for this in the theme repo once this PR is merged.
Agreed, both that issue and the missing CSS variables are unrelated to this PR so let's look into those separately.
LGTM!
4faf3ab
to
e7bc4a5
Compare
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.
This Gutenberg support issue showed up: https://wordpress.org/support/topic/v11-8-0-color-issues/ |
Could it be related to the change in Cover block: Only use white text when the background of the cover block is dark. It sets some default colors, most notably, black where the background "is light". An For example, selecting a black background in Gutenberg 11.7 gives me black text in the editor, but white on the frontend:
In Gutenberg 11.8:
cc @scruffian |
I think that's the expected behaviour. cc @glendaviesnz |
@ramonjd That is correct - #33541 was designed to get around the issue of someone adding a cover block with a dark background and then typing in paragraph and not seeing anything as text colour black, so it takes a stab at setting a visible text colour based on background being dark or not: |
Description
Currently it is not possible to specify opacity on a cover block when a background colour is selected, which prevents the use of some interesting design patterns as noted here.
This PR is experimenting with a slightly different approach to the one taken at #33927, as there were issues with that one with using a
style="opacity...
due to kses limitations.This uses the approach suggest by
@justintadlock of using the
span
already used by the gradient setting and keeping the structure more consistent.Fixes: #32339
Testing
Screenshots