-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Color system] add theme provider readme #2756
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
Conversation
🟢 No significant changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content looks great, thanks for getting this going so quickly. Some suggested edits and a couple questions.
|
||
### Theme provider rendered by the app provider | ||
|
||
The app provider component renders a theme provider component and a theme. By default, theme falls back to the current design language. The new design language is enabled by passing `{newDesignLanguage: true}` to the `features` prop on the app provider component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we might want to move away from using "current" and "new" when talking about design language. Could this be confusing and will it need to be changed in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a temporary problem, as this content pretty much goes away when the new design language is the design language. My vote is this stays as-is for now, but I am open to any language that can add some clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. What if we keep it but change "current" to "old" to be sure that folks don't mistake "current" for "new"?
There was a Github outage earlier and some weird race condition on Github's end happened, that's why there's two splash comments above. I went ahead and deleted the stale one, sorry for the confusion friends |
c90a44a
to
12d416c
Compare
This is ready to ship, but we'll need to make sure that the style guide does not wrap these examples in an app provider, as it will break the examples. |
7be18f8
to
0bc9871
Compare
- color scheme | ||
- light mode | ||
- dark mode | ||
omitAppProvider: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the secret sauce for turning off the wrapping AppProvider
in Storybook and the Style Guide
|
||
## Consuming theme colors in a component | ||
|
||
The theme provider component uses [CSS custom properties](https://developer.mozilla.org/en-US/docs/Web/CSS/--*) to share color values with components. For a full list of available CSS custom properties, see the [Polaris tokens docs](link/to/polaris/tokens/docs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this link is broken which causes the Style Guide build to fail (link checker)—might require a patch release. For now this can point to https://github.com/Shopify/polaris-react/blob/master/documentation/Color%20system.md.
Todo