Cover block :Add back missing styles #38362
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Type] Bug
|
Size Change: +1.37 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Jan 31, 2022
| } | ||
| } | ||
| } | ||
|
|
||
| @for $i from 0 through 10 { | ||
| .wp-block-cover__gradient-background.has-background-dim.has-background-dim-#{ $i * 10 } { |
There was a problem hiding this comment.
Just curious: what's the significance of the classname wp-block-cover__gradient-background? It's added to the cover blocks' background dim element regardless of whether the overlay color is a gradient.
There was a problem hiding this comment.
As far as I am aware it is only added if there is a gradient background, and so using this rule if that class does not exist the opacity is applied to the ::before of this element, otherwise it is applied to the element itself ... but maybe I will do some additional testing to check that assumption 🤔
There was a problem hiding this comment.
Wondering if the span needs something like 'wp-block-cover__gradient-background': !! gradientValue, in the edit and save.js.
Deprecations aside... 😄
ramonjd
approved these changes
Jan 31, 2022
|
FYI - we need to do some more testing to make sure that gradient backgrounds are handled properly before this is merged. |
Gradients are working as expected for me in the frontend and in the editor.
The only thing I'm seeing is that, as mentioned above, the It's not necessarily a problem and not blocker for this PR it appears. The only issue I'm currently seeing is one of nomenclature: the class is being applied where there is no gradient, which might be confusing for anyone who looks or relies on the class. The CSS changes here compound the "is state" by targeting the classname. I think we should remove or rename it to avoid problems down the line. Perhaps rename it to Edit: another peculiarity we have are duplicated I've started something over here: #38392 Deprecations are pretty tricky. Would be good to get your advice on whether we need/should do this. Maybe I'm missing something with the updated classnames. |
8 tasks
|
@ramonjd gradient opacity seems to work as expected for unmigrated blocks as well. I have added additional markup in the description which includes gradient background, default background and custom background colours, and all seem to display as expected in the frontend with this patch. |
|
This is testing fine for me still, thank @glendaviesnz The opacity of the
Just noting that when switching from Text to Visual the Classic Editor strips the background span in first test block, so the gradient and the dim ratio disappear - that caused me a bit of confusion. The trick for testing is not to switch 😄 |
andrewserong
approved these changes
Feb 1, 2022
There was a problem hiding this comment.
Thanks for adding this back in @glendaviesnz, and for the good testing instructions! The legacy markup is working correctly with the added styles back in, and I tried a few different levels of opacity to double check they're all working, both with the old markup and inserting a new cover block:
It's a little bit of a shame that we need to keep around older styling for backwards compatibility (would it be worth adding a comment above the added back in styling with a link to the reported issue, just in case?), but it's definitely better for us to re-instate it, and it doesn't look like it causes any conflicts with the newer markup.
Also, I double checked that the code is the same as what was removed in #35065 — so, it looks good to me!
Good idea @andrewserong , I have added a comment. I would like to refactor the css to pull all the deprecated selectors into a |
Agreed, a separate PR for refactoring (that doesn't need backporting) sounds like the way to go 👍 |
|
An additional note for anyone waiting for this to go out in 5.9.1 - adding this to site custom CSS should fix the issue in the meantime. |
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
* Add back missing styles to ensure existing cover blocks show correction dimRatio Co-authored-by: Glen Davies <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Since a change to how Cover block handles background opacity, old block content has been defaulting to 0.5 opacity, instead of using the dimRatio that is set in the block (reported here https://core.trac.wordpress.org/ticket/55000).
Testing Instructions
Install Classic editor on a site so you can paste the following into a post without it being migrated:
View the post in the frontend and make sure black overlay over image is set to 0.2 and not 0.5 for gradient, default background and custom background colour.
Screenshots
Before:
After:
