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

Try: Use currentColor to design quote blocks #26684

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Nov 4, 2020

This PR reverts changes in #26510 and resurrects changes in #25358. It changes Quote and Pullquote blocks to use currentColor to colorize both the content and citation fields, ensuring that it's legible in the editor, and on the frontend:

currentcolor

There is also #26686 as an alternative to this one. The benefit of this PR is that it fixes both the issue with input field placeholder legibility, and also makes both quote blocks more useful in dark themes.

@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Block] Quote Affects the Quote Block [Block] Pullquote Affects the Pullquote Block labels Nov 4, 2020
@jasmussen jasmussen self-assigned this Nov 4, 2020
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

Size Change: -90 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-library/theme-rtl.css 792 B -45 B (5%)
build/block-library/theme.css 793 B -45 B (5%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 131 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 9.06 kB 0 B
build/block-library/editor.css 9.06 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.9 kB 0 B
build/block-library/style.css 7.9 kB 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.5 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/index.js 22.4 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jasmussen jasmussen requested a review from kjellr November 4, 2020 10:51
@jasmussen
Copy link
Contributor Author

Kjell my friend, I know that this one has been by you in the past, and that we decided to shelve it. But I'm resurrecting it for two reasons:

  1. It solves some of the issues around input field placeholder text as outlined in Update the .is-dark-theme UI for Quote and Pullquote blocks #26510
  2. It only touches theme.scss files, which require opt-in, which I feel in hindsight makes this a little less disruptive.

It's also the cleanest way to address the various moving parts. Would love your thoughts once more if you have time.

@jasmussen jasmussen force-pushed the try/quotes-currentcolor branch from 762ee11 to 915e5bc Compare November 4, 2020 12:35
@kjellr
Copy link
Contributor

kjellr commented Nov 4, 2020

My concern is still just that this will change the display of quotes on the front-end of people's existing sites. 😕 Is it possible to make this change only when a dark theme is active? I'd feel a lot better about it then.

Either way, as you noted this is a recurring issue. I think it might be worth trying this change out — we can always revert if it seems to be a problem for folks.

@jasmussen
Copy link
Contributor Author

Thank you for chiming in. The issue is that we can't know if a theme is dark or not on the frontend. There's no mechanism to discern that, and I don't think there will be.

@kjellr
Copy link
Contributor

kjellr commented Nov 4, 2020

Ah that's right. I see what you mean. 😕

Then yeah, I say we go with option two: push this for now, revert/iterate later on if it's a problem.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

I just tested it and it looks good. 👍 LGTM

@jasmussen
Copy link
Contributor Author

Let's try this, and I will revisit yet again if we get any issues.

@jasmussen jasmussen merged commit 382d5df into master Nov 4, 2020
@jasmussen jasmussen deleted the try/quotes-currentcolor branch November 4, 2020 18:55
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pullquote Affects the Pullquote Block [Block] Quote Affects the Quote Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants