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

Fix regression with placeholder text color. #10288

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
2 participants
@jasmussen
Contributor

jasmussen commented Oct 2, 2018

Recently we increased the contrast of text color in placeholder text.

As part of this, we changed from using opacity, to using rgba, to provide not-fully-opaque placeholder text. This caused a regression in contexts that did not have a light background, because the RGBA obviously adds a color, it doesn't just change the opacity.

For example placeholder text in a Cover Image block was suddenly dark, and if you added a dark editor style, placeholder text was not legible.

This PR changes that back, with a few tweaks, so it relies on opacity again. The benefit to this is that any block or editor style that provides its own text color for elements can use the placeholder aspect without having to also worry about the placeholder text color in various background environments.

This PR ensures there is sufficient (4.7+ contrast ratios) for placeholder text in:

  • Cover Image
  • Image captions
  • Empty paragraphs in dark editor styles

screen shot 2018-10-02 at 10 58 37

screen shot 2018-10-02 at 11 01 47

screen shot 2018-10-02 at 10 58 32

Fix regression with placeholder text color.
Recently we increased the contrast of text color in placeholder text.

As part of this, we changed from using opacity, to using rgba, to provide not-fully-opaque placeholder text. This caused a regression in contexts that did not have a light background, because the RGBA obviously adds a color, it doesn't just change the opacity.

For example placeholder text in a Cover Image block was suddenly dark, and if you added a dark editor style, placeholder text was not legible.

This PR changes that back, with a few tweaks, so it relies on opacity again. The benefit to this is that any block or editor style that provides its own text color for elements can use the placeholder aspect without having to also worry about the placeholder text color in various background environments.

This PR ensures there is sufficient (4.7+ contrast ratios) for placeholder text in:

- Cover Image
- Image captions
- Empty paragraphs in dark editor styles

@jasmussen jasmussen added this to the 4.0 milestone Oct 2, 2018

@jasmussen jasmussen self-assigned this Oct 2, 2018

@jasmussen jasmussen requested review from mtias, iseulde and WordPress/gutenberg-core Oct 2, 2018

@tofumatt

Looks good, if you could just tweak the comments then it's good to merge. 👍

.is-dark-theme & {
color: $light-opacity-300;
}
opacity: 0.62;

This comment has been minimized.

@tofumatt

tofumatt Oct 2, 2018

Member

That's an awfully specific percent 😆

@tofumatt

tofumatt Oct 2, 2018

Member

That's an awfully specific percent 😆

This comment has been minimized.

@jasmussen

jasmussen Oct 2, 2018

Contributor

Yep ;)

It's from here: https://github.com/WordPress/gutenberg/blob/master/edit-post/assets/stylesheets/_colors.scss#L36 — which is part of the array of opacity based colors which were very carefully put together in #6937

In other words, we're using the same opacity as the previous RGBA variable does. I did try rounding up, but that lost a surprising amount of contrast, in both directions.

@jasmussen

jasmussen Oct 2, 2018

Contributor

Yep ;)

It's from here: https://github.com/WordPress/gutenberg/blob/master/edit-post/assets/stylesheets/_colors.scss#L36 — which is part of the array of opacity based colors which were very carefully put together in #6937

In other words, we're using the same opacity as the previous RGBA variable does. I did try rounding up, but that lost a surprising amount of contrast, in both directions.

Show outdated Hide outdated packages/block-library/src/gallery/editor.scss Outdated
Show outdated Hide outdated packages/block-library/src/image/editor.scss Outdated
@@ -94,12 +94,10 @@
pointer-events: none;
// Use opacity to work in various editor styles.
// We don't specify the color here, because blocks or editor styles might provide their own.

This comment has been minimized.

@tofumatt

tofumatt Oct 2, 2018

Member

Very helpful 👍

@tofumatt

tofumatt Oct 2, 2018

Member

Very helpful 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2018

Contributor

Thanks for the review @tofumatt. In 020b360, I pushed what's more or less a refactor inspired by your feedback. Essentially I realized that whether it's a caption that's underneath an image (gray text) or overlaid on a gallery item (white text on black scrim), the same .8 opacity is necessary. Not only that, but also embeds and other things have captions.

So I made a single generic rule for all caption placeholders, that seems to work. Can I get a sanity check/re-review? Thanks.

Contributor

jasmussen commented Oct 2, 2018

Thanks for the review @tofumatt. In 020b360, I pushed what's more or less a refactor inspired by your feedback. Essentially I realized that whether it's a caption that's underneath an image (gray text) or overlaid on a gallery item (white text on black scrim), the same .8 opacity is necessary. Not only that, but also embeds and other things have captions.

So I made a single generic rule for all caption placeholders, that seems to work. Can I get a sanity check/re-review? Thanks.

@tofumatt

Looks sane to me! 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2018

Contributor

Thank you!

Contributor

jasmussen commented Oct 2, 2018

Thank you!

@jasmussen jasmussen merged commit ff12c94 into master Oct 2, 2018

2 checks passed

codecov/project 49.44% remains the same compared to 610aa4e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/placeholder-regression branch Oct 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment