-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Enable opacity control (transparency) in blocks that support background color #25421
Enable opacity control (transparency) in blocks that support background color #25421
Conversation
I am not sure whether the failing tests are related or how I can fix them? Any help will be appreciated! |
I think this could be cool to support but since today we save colors in the hex format, I'm not sure if this has impact on backward compatibility or not. (validation...) |
Thanks @youknowriad for chiming in! That is a valid point! I just tried creating a block in the master branch and then edited it in the current branch without any problems. Since the color type (hex/rgb) is changed when Even doing the opposite, creating a block with alpha and then moving to a branch that doesn't support alpha, didn't produce any errors. It fallbacked to hex. |
I like this initiative a lot, just recently I was working on a mockup of an improved Cover block toolbar that doesn't change quite as much between color-only and media states. As part of that, it felt natural to add an opacity control to the color block, because there is when the color is used as an image overlay. Work in progress: This also touches on Riad's comment:
In that vein, instead of changing the RGB color to be RGBa, is it possible to refactor the cover block to use RGB plus an opacity instead? I know this is a bigger change, but it would bring some consistency between the color-only version and the media versions of the cover blocks. |
There have been multiple attempts in the past to enable this, all of them got blocked by one thing or the other. I hope this time we make it happen. This PR will expose the alpha picker to users and I'm not entirely sure it has been properly reviewed for a11y... I added the needs-a11y-feedback tag. |
There's already an opacity picker if you have an image background. And there's also an alpha picker for each gradient step, if you choose a gradient background. Fine to review either of those if they haven't already been, but it shouldn't be new, unless I'm missing something. |
Ah you're right, I hadn't noticed the ones in gradients, I only remembered the ones for background-images... and those opacity controls are not part of colorpickers (which is why I had the impression these here might need an OK from an a11y standpoint) 👍 |
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.
@cpapazoglou Awesome! Thanks for working on this one. 👍 from me from a code implementation perspective :).
Note: There's some input value rendering funkiness within the ColorPicker (for alpha). That is an existing issue and not related to this PR.
Worth just being sure about this one before we merge: #25421 (comment) — I think there's a way to refactor cover to not need the change to rgba as noted in #25421 (comment). |
This would require:
If we need to consolidate things, I would remove the opacity setting from media and let only the rgba for media / background-color / gradient |
@jasmussen I pushed 2afee4d that refactors cover block to allow If yes, I will have to apply some changes for the frontend too!
|
This is what I see: And yes, I think this is what I'm after! We entirely avoid having to use rgba, and the featureset seems to be the same. It also brings consistency to how gradient overlays are created, so it's the same UI. That's a real win. Nice work. As you can see from the GIF, there's a weird thing that happens to the opacity when it hits 50% exactly. I'm not entirely sure what happens there. I would also love a sanity check by @jorgefilipecosta as I recall he worked on this. Thank you! |
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 here thank you for all the changes applied the alpha picker seems to be working well and it seems a safe change to me. I just left a minor suggestion related to onChangeComplete handling.
Regarding the changes for cover, in my tests things seem to be working as expected on the frontend. There were some discussions in the past related to moving cover image background to a dom element if we in fact are going to do that it may make sense to do the change we are doing here to cover and that change together to avoid multiple deprecations and more importantly multiple CSS support for old versions which on the cover block is already complex.
disableAlpha | ||
onChangeComplete={ ( color ) => | ||
onChange( | ||
disableAlpha ? color.hex : rgba( color.hex, color.rgb.a ) |
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.
I think when alpha is not set we should store the colors in hexadecimal, even if picking alpha is enabled. The onChange condition should not be based on alpha being enabled or not but on alpha being set or not.
@@ -10,6 +10,7 @@ | |||
justify-content: center; | |||
align-items: center; | |||
padding: 1em; | |||
background-color: transparent; |
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.
These style changes are causing problems for existing cover blocks.
In all cases where we have an image or video background and an overlay color or gradient. With these changes, the overlay color and gradients stop appearing on the frontend of the websites.
To reproduce one can add some cover blocks select image, video backgrounds, and overlay color /gradients save the post and see it on the front end. The overlays will not be there.
@@ -515,14 +516,24 @@ function CoverEdit( { | |||
src={ url } | |||
/> | |||
) } | |||
{ url && gradientValue && dimRatio !== 0 && ( | |||
{ overlayColor.color && ! gradientValue && dimRatio !== 0 && ( | |||
<span |
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.
We are adding this span element to the edit function but not to the save markup that the block produces and is shown on the frontend. Is the span also required on the frontend? If yes, I guess we may need to add it to the save function, but that is complex:
- It will require CSS that works with the new block markup and with the old one.
- It will require block deprecations.
Is there any way we can accomplish the same without requiring the additional span?
A possible way would to when there is an opacity value set, and there's a color change, we would add the alpha value to the color attribute even though in the color picker alpha does not appear. It is a little bit hacky and makes one attribute depend on the other, but that's only on editing on the frontend; no changes are required. For the gradients, I guess our gradients library would need to expose some function that allows changing every stop of the gradient to a specific alpha. This solution is also a little bit complex; the main advantage is that the dom we produce would be simpler as we would not require a dom element just for a color background.
In the future, we will probably move the image background to a dom element too. To take advantage of the server-side processing, WordPress does for images. If we follow that refactor, I wonder if it would make sense to join this work and that image change to avoid the need to add deprecations two times and the need to have our CSS code support more two different versions of the markup. In that case, I think it may make sense to not do the change to cover yet, and then do a wide refactor to the cover block with all these changes that are pending.
What do you think @jasmussen?
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.
In the future, we will probably move the image background to a dom element too. To take advantage of the server-side processing, WordPress does for images. If we follow that refactor, I wonder if it would make sense to join this work and that image change to avoid the need to add deprecations two times and the need to have our CSS code support more two different versions of the markup. In that case, I think it may make sense to not do the change to cover yet, and then do a wide refactor to the cover block with all these changes that are pending.
This sounds very reasonable. As far as I'm aware, there are many pros to moving to a regular image, including around how srcsets work. The primary challenge, as I perceive it, is to refactor how the fixed position works, where currently it's a simple background-position change, whereas were it an image, it'd probably need to combine some position: fixed;
with overflow: hidden;
on a parent.
CC: @ajlende as I believe you've been involved in related work.
In the mean time, I would agree that it'd be best to find a way to avoid block deprecations. I have some thing to look into elsewhere today, but when I have a moment, I'm going to try and dive into the PR and see how I may be able to help.
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.
@jorgefilipecosta The work for moving the cover block background image is in #25171. It's just kinda sitting there waiting for a +1.
That PR doesn't include changes for the fixed or "parallax" mode that Joen is referring to, so if we're trying to limit the number of deprecations, we'd also probably want to include that change. I can add it to #25171 or we can open another PR to review it separately and join the three together before merging them all.
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.
@ajlende just to be sure I'm understanding you correctly, #25171 removes the "parallax" feature from the cover block, correct? I guess that makes sense insofar as now that I think about it, it's really tricky to replicate with an img
, because position: fixed;
puts it outside of the containers stacking order, so it can't be "cropped" by the cover container.
But that's still a big bummer. Is there some basic parallax JavaScript that we can use to replicate the parallax effect, without deprecating it?
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.
#25171 didn't touch the parallax feature—choosing parallax sets the background-image
property as before and is the only option that doesn't use srcset
. That's what I was referring to when I was talking about opening a third PR—to see if it's worth trying to find a JS solution which will allow both srcset
and the parallax effect.
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.
Oh that's smart! That seems totally fine. Thank you for the clarification.
I tested with the Cover block and this works nicely! |
Seems like a feature that would be best made available via Block Support/theme.json, no? |
Great suggestion @stokesman, we can create a |
Yeah, that's the idea. Though on second thought I wonder if it'd be important to have the support flag. I'm not sure that theme creators would feel the need to control this separately. There's already the custom color supports setting and disabling that would also disable the access to the alpha control. |
If we intend to go through with this, the PR should be rebased 'cause it's been a long time and there are numerous merge conflicts now 👍 |
2afee4d
to
3290dbb
Compare
I started this PR months ago. |
I think we can close this one now that we have alpha controls in the colorpicker. See: #37731 If I'm mistaken, please reopen. Thank you! |
Description
Fixes #18095
disableAlpha={ false }
toPanelColorGradientSettings
component of any block which in turn allows for transparency (alpha) in that color.backround-color
of:cover
,button
,paragraph
,heading
,list
How has this been tested?
Overlay
and select custom colorScreenshots
In Action
![SS 2020-09-18 at 15 37 48](https://user-images.githubusercontent.com/12430020/93598172-fd7dae80-f9c4-11ea-9ee9-cd1d8078fef5.gif)
Types of changes
New feature
Checklist: