-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update overlay step function in cover block #59891
base: trunk
Are you sure you want to change the base?
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Hi @Mamaduka, need your help with PR review here. |
Hi, @sunil25393 It's been a while since I worked on the Cover block, and I might miss some of the pre-requestions needed for this change. I would recommend contacting folks who worked on the overlay feature "recently". cc @t-hamano, @stokesman P.S. Removing the |
I have kept the css for backward compatibility. We can deprecate it in future version of gutenberg i guess. |
Yes but George was referring to its corresponding class in the markup. There could be instances of CSS written using it in their selectors. Here’s what can be found in themes from wpdirectory. Most of the usage seems odd to me and I'd like to think we can not worry about breakage but I think that’d be against policy. Otherwise, I gave this a test drive and it worked well. I only tested a basic Cover block created from trunk before switching to this branch and the block updates successfully. I'm not sure how else these types of changes need to be tested. |
Thank you for your reply. I see it now. However, if we leave the class selectors, opacity will be applied twice. There is no significance to it. Is there a workaround for this? We can always update the changelog, though. |
I took a closer look at the selectors used in the themes from that search and all of them that I saw are written for older cover block versions which had the Yet I didn’t examine all the results because they are many (though it appears they are all repetitions of what I did examine). I wanted to try a more exacting regex to find results that aren’t paired up with I've also ran the search on plugins. There are some results though it appears that they are for custom blocks that adopted the use of the class and so should be unaffected by this change. For example, I tested the most installed plugin in the results (WooCommerce) and the class is used on their Featured Product block that has nothing to do with Cover. My feeling is that this should be safe to ship as is but I don’t know the level of caution others might think is warranted. |
Thank you for investing @stokesman, @t-hamano can we get your view on this ? |
@WordPress/outreach Need this PR to be reviewed. |
Hi @akasunil You need to update the PR with the latest files from Gutenberg trunk, and make sure that the tests pass. |
Change to 6.5 compatibility files need to be moved, I assume since we are in RC already, this needs to be in a 6.7 compatibility file, not 6.6. |
Hi @carolinan, I have updated PR. Could use review. |
Original PR from Gutenberg repository: * [WordPress/gutenberg#59891 #59891 Update overlay step function in cover block] Reference: [https://developer.mozilla.org/en-US/docs/Web/CSS/opacity MDN Web Docs: opacity]. Props sunil25393, wildworks, poena, Mamaduka, presstoke. Fixes #61536. git-svn-id: https://develop.svn.wordpress.org/trunk@58709 602fd350-edb4-49c9-b593-d223f7449a82
Original PR from Gutenberg repository: * [WordPress/gutenberg#59891 #59891 Update overlay step function in cover block] Reference: [https://developer.mozilla.org/en-US/docs/Web/CSS/opacity MDN Web Docs: opacity]. Props sunil25393, wildworks, poena, Mamaduka, presstoke. Fixes #61536. Built from https://develop.svn.wordpress.org/trunk@58709 git-svn-id: http://core.svn.wordpress.org/trunk@58111 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Original PR from Gutenberg repository: * [WordPress/gutenberg#59891 #59891 Update overlay step function in cover block] Reference: [https://developer.mozilla.org/en-US/docs/Web/CSS/opacity MDN Web Docs: opacity]. Props sunil25393, wildworks, poena, Mamaduka, presstoke. Fixes #61536. Built from https://develop.svn.wordpress.org/trunk@58709 git-svn-id: https://core.svn.wordpress.org/trunk@58111 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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.
Thank you for all the hard work on this PR @akasunil 👍
I'm still in the process of giving the code a thorough review and test but thought it important to share a few early thoughts.
Backwards Compatibilty
As has already been discussed on this PR, the removal of the dim ratio CSS class from the block will cause existing blocks to be flagged as invalid due to the saved markup no longer matching what is generated in the editor.
If the dim ratio CSS class is to be removed in favour of an inline opacitiy style, the Cover block will need a deprecation to migrate the old versions' markup.
A lot of the block fixtures that are updated in this PR are related to past deprecations. Changing them means they don't match the markup those deprecated versions of the block actually have so are now incorrect.
The non-__deprecated
fixtures could be updated with the proposed inline opacity styles but we would also need new fixtures covering the deprecation of the dim ratio class.
Alternative Approach
There are also a number of open issues currently to update the Cover block such that it's overlay is configurable via theme.json and Global Styles. The primary issue for this is:
The general idea is that the background color block support could be used to provide the color for a Cover block's overlay. This block support also allows defining opactity via the color's alpha channel.
It's been on my radar for a while but I haven't had the bandwidth in the lead up to 6.6 to explore too deeply. I believe @ramonjd has also started working on this too.
I'm not sure yet whether the UI for the Cover block's overlay opacity would be removed so we rely solely on the background color control or whether it would be left in place as a convenience until it can be deprecated.
With the release of the Section Styles feature, the ability to style the Cover block's overlay via theme.json is getting more attention. I'd go so far as to say it's a must-have. (cc/ @richtabor)
As this PR won't address the above need and the alternative approach would also solve the main concern this PR addresses (i.e. finer control over opacity). I'd like to propose we avoid introducing the inline opacity style which would require yet another deprecation once the overlay is switched to use the background color block support.
I really do appreciate the huge amount of work that has gone into this PR. FWIW it might still help inform the direction we ultimately go in.
If you are open to it, perhaps we can collaborate on the alternative approach?
Agreed. |
I tend to agree. With this effort in flux yet I don't think we should push forward on too many changes with the opacity control. I think perhaps it coexists, but I'd like to push that work forward first. |
Original PR from Gutenberg repository: * [WordPress/gutenberg#59891 #59891 Update overlay step function in cover block] Reference: [https://developer.mozilla.org/en-US/docs/Web/CSS/opacity MDN Web Docs: opacity]. Props sunil25393, wildworks, poena, Mamaduka, presstoke. Fixes #61536. git-svn-id: https://develop.svn.wordpress.org/trunk@58709 602fd350-edb4-49c9-b593-d223f7449a82
Thank you for the detailed feedback on this PR. I appreciate the time you're putting into reviewing. I agree that we should minimize the deprecation as much possible, If we have better way to improve functionality, we should discuss that. Coming to the alternative approach you proposed, which involves updating the Cover block such that its overlay is configurable via theme.json and Global Styles, is definitely better solution then the inline styles this PR add into block.
Just for the clarity, Does it mean we add a new setting for overlay color under block color support? I'm open to collaborate on the alternative approach. It sounds like a more flexible solution for the Cover block's styling capabilities.
Thank you again for your valuable insights and for recognizing the work put into this PR. |
Ideally, we'd reuse the background color support as is, if possible. From the UI side, I think the difficulty would be labelling the "background" control as "overlay" so its purpose is still clear.
That sounds great, thanks 👍 As that work progresses, I'll add you to any issues and PRs as they're created. |
It's probably fine with the first iteration to just use "Background" even. There's a few ideas floating around about consolidating background color and image into one PanelBody, it may be that we don't need to change the label if that's the case. |
Nothing recently, but it's worth highlighting the proposal to merge background controls in the UI. This wouldn't affect the way the background values are read and stored in theme.json, but would eventually make controls consistent across blocks that have background image and background color supports enabled, the Cover block (hopefully one day) included. |
What?
Fixes #51625
Why?
Currently, we can set overlay in multiple of 10 in cover block.
How?
I have updated steps to enable any number from 1 to 100 to be set as overlay value.
Testing Instructions
Screenshots or screencast