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

Import all used Gutenberg component styles. #2679

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@jeffstieler
Copy link
Contributor

commented Jul 19, 2019

Fixes postcss theme overrides.

Fixes #2322

This PR seeks to fix the postcss theme overrides by importing in the styles from Gutenberg into WooCommerce Admin.

Postcss-themes will not render out the theme style if it doesn't find corresponding usage in the SCSS it's processing. See: https://github.com/WordPress/gutenberg/blob/c7e5b403509f7bd0d577da48dc5f582b9f9f97a2/packages/postcss-themes/index.js#L12

Detailed test instructions:

  • Follow the test instructions on the issue
  • Verify that the toggle is Woo purple
  • Smoke test other areas of WooCommerce Admin to make sure styles are as expected

@jeffstieler jeffstieler requested a review from woocommerce/wc-admin Jul 19, 2019

@jeffstieler jeffstieler added this to In Progress PRs (for automation purposes) in wc-admin via automation Jul 19, 2019

@psealock
Copy link
Collaborator

left a comment

Thanks for tackling this one @jeffstieler. LGTM, we should 🚢 , but I explain below there is an opportunity to contribute upstream changes to Gutenberg here.

@import 'gutenberg-components/select-control/style.scss';
@import 'gutenberg-components/spinner/style.scss';
@import 'gutenberg-components/text-control/style.scss';
@import 'gutenberg-components/tooltip/style.scss';

This comment has been minimized.

Copy link
@psealock

psealock Jul 22, 2019

Collaborator

I tried adding @import 'gutenberg-components/form-token-field/style.scss'; because I saw form field tokens look off:

Screen Shot 2019-07-22 at 2 40 46 PM

And it led me down a path of adding variable after variable. It reminded me that adding variables here was simply a stop gap until CSS could be exported from Gutenberg independently. Otherwise, we'll be forever configuring these. More info here, WordPress/gutenberg#7716 (comment).

It seems like time to make an upstream contribution. Since we don't use these tokens (at the moment), we can leave as is.

This comment has been minimized.

Copy link
@jeffstieler

jeffstieler Jul 22, 2019

Author Contributor

I'm not grokking what the upstream contribution would be to export SCSS from Gutenberg based on that PR discussion. Is the change to move the styling to the JS files?

This comment has been minimized.

Copy link
@psealock

psealock Jul 22, 2019

Collaborator

No, but that was a potential solution. postcss-loader uses the postcss.config.js file to apply Woo branding to styles in Gutenberg repository. For example, here is a style from form token field:

.components-form-token-field__token-text {
    background: transparent;
    color: theme(secondary);
}

We need to explicitly @import 'gutenberg-components/form-token-field/style.scss' so that our build step can process the theme function call and apply Woo's secondary color. This requires that we also import all scss variables also found in the file, which is what this PR does.

At the very least, Gutenberg will export all its variables, includes, and mixins in a package so that we don't have to go through this process on every update to @wordpress/components and instead consume the exported variables at build time. At the very best, our build step builds and themes css at build time without us having to do anything, as would be the case with css in js.

@psealock

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

@justinshreve Does Onboarding require these themed components? If so, we should merge this. If not, we can focus on an upstream solution in Gutenberg, explained in #2679 (comment).

Import all used Gutenberg component styles.
Fixes postcss theme overrides.

@jeffstieler jeffstieler force-pushed the fix/2322-postcss-theme branch from 9086df7 to 5b7a0a0 Aug 14, 2019

@jeffstieler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Is this OK to be merged @psealock?

@psealock

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

@jeffstieler Yup, lets merge.

@jeffstieler jeffstieler merged commit 2c3eafc into master Aug 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

wc-admin automation moved this from In Progress PRs (for automation purposes) to Done Sprint 23 (August 13 - August 26) Aug 15, 2019

@jeffstieler jeffstieler deleted the fix/2322-postcss-theme branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.