Skip to content
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: improve overlay opacity handling #26133

Merged
merged 4 commits into from
Oct 19, 2020

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Oct 14, 2020

Fixes: #25290 which I believe was created by the change from #20904 as it moved the assignment of the black background color into a selector with greater specificity. That PR and its comments are also relevant here because in them an alternative fix was suggested by @jasmussen which is part of the changes in this PR. That is removing the styled fallback background color from the cover. (I'm not referring to it as the default background color because it's just styled and not part of the block attributes). The idea was discarded since it would "break" the dimming feature but if there's no background color or gradient set, that feature just shouldn't be available. Which is what has been done in this PR.

Besides that this gives the opacity range slider a step in accord with the available opacity styles 68419b4 and includes a very minor CSS optimization 0fb0fed.

Note: I'd say part of the issue in #25290 is that the has-background-dim class is inappropriately applied to the block. I initially started on a fix by improving the logic for when that class is applied. However, that leads to block validation failures. If there's a way to handle the migration/upgrade of a block then that'd still be a nice thing to fix.

Screen recording demonstrating availability of opacity control only with application of background color (also stepped opacity increments)

Also worth noting that the background image used in that recording would not even be visible without the changes in this PR.

How has this been tested?

WordPress 5.5.1, creating various Cover blocks with and without images and background colors.

Types of changes

Bugfix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@jasmussen
Copy link
Contributor

Very nice. This is my before:

before

And this is after:

after

And so as you can see, purely based on those GIFs, this PR solves that issue.

I also verified that this fixes the issue for the overlay aspect as well. Editor:

Screenshot 2020-10-15 at 10 41 42

Frontend:

Screenshot 2020-10-15 at 10 41 50

Currently all overlays are black 🙈 so this would be a nice PR to land!

@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Oct 15, 2020
@stokesman stokesman marked this pull request as ready for review October 15, 2020 16:05
Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, tested well. No problems 👍

@mkaz mkaz merged commit c5ea5a5 into WordPress:master Oct 19, 2020
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 19, 2020
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have introduced a regression, I've just filed an issue: #26545

}
&.has-background-dim::before {
content: "";
background-color: inherit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have become $black because the inherited background-color was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that were to be $black themes would have to write some quite different styles to have their colors apply on Covers. The removal was motivated by pretty much that issue #25290, the way it made theme background style selectors need extra specificity. As an example, from twentytwenty:
:root .has-primary-background-color { ... }
Whereas the docs on defining those use a simple class selector.
Also, the inherit here is for when there's a defined background color via the block attributes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a "fix" in #26564, that I'd love your thoughts on.

sirreal added a commit that referenced this pull request Oct 29, 2020
sirreal added a commit that referenced this pull request Nov 2, 2020
@sirreal sirreal mentioned this pull request Nov 2, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom Cover Block overlay color overwritten by "has-background-dim"
4 participants