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

(JS) Color Values: Rename brand to theme. Add fallback values. #24059

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Jul 20, 2020

Screen Shot 2020-07-20 at 11 07 24 AM

This PR is a follow up to #23936

It fixes an IE11 regression for CSS variable support. It also renames the color ui.brand usage to ui.theme, which is feels more appropriate for this project.

How has this been tested?

  • Tested locally (Storybook + Gutenberg)
  • Run npm run dev
  • Add a Column block
  • Ensure the slider renders correctly

Types of changes

At the moment, we have to manually add fallbacks for any CSS var() usage (for CSS-in-JS generated styles). The Sass workflow offers automatic fallback generation thanks to it's PostCSS process. I recognize this isn't a great solution (especially at scale).

We'll need to look into a more automated solution for CSS variable fallbacks (as well as other features like RTL handling).

Checklist:

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

@ItsJonQ ItsJonQ added the [Package] Components /packages/components label Jul 20, 2020
@ItsJonQ ItsJonQ requested a review from youknowriad July 20, 2020 15:13
@ItsJonQ ItsJonQ self-assigned this Jul 20, 2020
@@ -42,7 +42,7 @@ function RangeControl(
beforeIcon,
className,
currentInput,
color: colorProp = color( 'ui.brand' ),
color: colorProp = color( 'ui.theme' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why not all use cases have fallbacks like this one?

Copy link
Author

Choose a reason for hiding this comment

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

This value is (eventually) passed into the styled generated component, which has a fallback:
https://github.com/WordPress/gutenberg/pull/24059/files#diff-bc2454ef211a3ab799a3f1c4dfe936c1R36

This workflow isn't ideal 🤦‍♀️ . I'm looking into an automated solution now.
I have something in mind 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that automating would be good or at least a lint rule. But I'm willing to include this fix quickly if you think it's ready as is?

Copy link
Author

Choose a reason for hiding this comment

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

It is ready as is 👍

@github-actions
Copy link

Size Change: +74 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/components/index.min.js 200 kB +74 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.min.js 125 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.58 kB 0 B
build/block-library/editor.css 7.57 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.77 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.68 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.min.js 16.8 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.min.js 9.37 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.3 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 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.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 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.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Browser Issues Issues or PRs that are related to browser specific problems labels Jul 20, 2020
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Let's get the fix in and explore automation.

@youknowriad youknowriad merged commit ac58042 into master Jul 20, 2020
@youknowriad youknowriad deleted the fix/js-color-values branch July 20, 2020 15:44
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 20, 2020
@ellatrix ellatrix mentioned this pull request Jul 20, 2020
6 tasks
@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 20, 2020

Automatic CSS var() handling incoming in this branch:
https://github.com/WordPress/gutenberg/tree/try/styled-provider-css-var-handling

@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants