WIP: [Block Library - Cover]: Add opacity to every possible background #33927
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
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
youknowriad,
mtias,
aristath,
talldan,
gziolo,
ajlende,
ramonjd,
carolinan and
jorgefilipecosta
|
Size Change: -736 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
This is what I see:
I can also nest things:
As far as I can tell, this seems to be working well. Was there anything in particular you wanted feedback on with regards to style tweaks or use cases? Nice work as always. |
|
Thanks for working on this feature! I just took it for a spin. Adjusting the opacity changes the opacity of the span, and I could create some cool effects, especially where the theme did not explicitly set the background color of the cover block itself (twentytwentyone does). For example, reducing the opacity of a cover block background color over a site background image:
I think this will be very useful combined with nesting and site background images. I know it's WIP but I threw an existing block at the deprecations and it was stripping the inline spacing and min-height styles. Here's the block code generated from trunk: <!-- wp:cover {"overlayColor":"black","minHeight":363,"className":"is-style-twentytwentyone-border","style":{"spacing":{"padding":{"top":"107px","right":"107px","bottom":"107px","left":"107px"}}}} -->
<div class="wp-block-cover has-black-background-color has-background-dim is-style-twentytwentyone-border" style="padding-top:107px;padding-right:107px;padding-bottom:107px;padding-left:107px;min-height:363px"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","textColor":"white","fontSize":"large"} -->
<p class="has-text-align-center has-white-color has-text-color has-large-font-size">Cover block pre-opacity</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover --> |
7 tasks
ntsekouras
force-pushed
the
add/cover-opacity-to-every-background
branch
from
3fcc99c
to
1cfb9ff
Compare
ntsekouras
force-pushed
the
add/cover-opacity-to-every-background
branch
from
1cfb9ff
to
7dd203e
Compare
jorgefilipecosta
approved these changes
Aug 12, 2021
There was a problem hiding this comment.
Hi @ntsekouras, thank you for working on this task 👍 I left some suggestions but generally this seems to be on a good path.
| ! customGradient && ! overlayColorClass | ||
| ? usedColor | ||
| : undefined, | ||
| opacity: dimRatio / 100, |
There was a problem hiding this comment.
Unfortunately, opacity is not a property that we currently allow for unprivileged users without unfiltered output capability so for example for contributor rules the styles will not be saved correctly.
The allowed properties can be checked at https://github.com/WordPress/wordpress-develop/blob/efe00618ab090fb14327f2e54bc670ecb2639ed4/src/wp-includes/kses.php#L2185.
We can consider a kses expansion if needed but in that case, we should do that in a different PR so the security implications can be discussed.
There was a problem hiding this comment.
I wish I knew that sooner 😢
| @@ -73,7 +70,6 @@ export default function save( { attributes } ) { | |||
|
|
|||
| const classes = classnames( | |||
| dimRatioToClass( dimRatio ), | |||
There was a problem hiding this comment.
We are still including classes like has-background-dim-90 and then we further add inline styles with opacity:0.9. Could we update the classes to add the opacity to the span element and avoid the inline style? It would also solve the kses issue.
| * We default to `black` background to enable the opacity of | ||
| * images/videos when we haven't explicitly set any overlay color. | ||
| */ | ||
| const usedColor = overlayColor || customOverlayColor || 'black'; |
There was a problem hiding this comment.
The default to black makes sure even if the user did not select any color we are going to include an inline style "background-color:black". Could we apply this default using CSS as we were doing before and avoid an inline style by default?
| return ( | ||
| <div { ...useBlockProps.save( { className: classes, style } ) }> | ||
| { url && ( gradient || customGradient ) && dimRatio !== 0 && ( | ||
| { dimRatio !== 0 && ( | ||
| <span | ||
| aria-hidden="true" | ||
| className={ classnames( | ||
| 'wp-block-cover__gradient-background', |
There was a problem hiding this comment.
We are using this span even for colors now. Should we include a general class for this element e.g: wp-block-cover__background, and only include the old wp-block-cover__gradient-background on gradients for back-compatibility?
We may also discard the class old class and include migrations or introduce a new class wp-block-cover__color-background that is used for colors keeping wp-block-cover__gradient-background on gradients.
| @@ -63,16 +49,6 @@ | |||
| opacity: 0.5; | |||
| } | |||
|
|
|||
|
|
|||
| @for $i from 1 through 10 { | |||
| &.has-background-dim.has-background-dim-#{ $i * 10 } { | |||
There was a problem hiding this comment.
We can not delete these classes, otherwise, for current blocks using these classes during the next WordPress update the classes will not be present and the blocks visual will change on the frontend.
jorgefilipecosta
requested changes
Aug 12, 2021
|
Thanks for reviewing @jorgefilipecosta ! The remaining problem I was trying to solve was about backwards compatibility with |
|
I'm not sure how to approach this in a way that works as expected and keep backwards compatibility with the existing markup and css styling.. Previously the background-color was handled in the main wrapper with css classes which means:
If anyone has some ideas, please share them 😄 |
|
I hope this fits with this issue but there is another edge case that might fit in here: Disabled colors for the cover block. Currently if you have color support for the cover block disabled (e.g. via |
|
The approach suggested here seems to work, but I now need to do some work on untangling the backwards compat/deprecation/migration issues of this approach. |
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
Resolves: #32339
This PR tries to add
opacityhandling inCoverblock with any background (image/video or bacgroundColor/gradient). I've approached this by keeping the existingspan.wp-block-cover__gradient-backgroundand showing it always ifdimRatiois applied - which should be always the case as we have a default value for this attribute.I've removed some styles that were handling the opacity and added the opacity right into this
spanoverlay layer. This way it makes nestedCoverblocks appear as expected.Testing instructions
Coverblocks and try differentopacitysettings and combinations.Notes
deprecatedversions some extra attributes that didn't make sense likecontentAlign and titlewhere I removed them from some deprecations. This seemed to me that they were there only by somecopy/pasteof attributes in some new deprecations.Screenshots
--cc @justintadlock
Checklist:
*.native.jsfiles for terms that need renaming or removal).