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

[next] Add multi-theme support to Polaris #10423

Merged
merged 14 commits into from
Sep 20, 2023
Merged

Conversation

aaronccasanova
Copy link
Member

@aaronccasanova aaronccasanova commented Sep 12, 2023

Continuation of #10290

This PR contains breaking changes in both Polaris and Polaris tokens to enable global multi-theme support.

Polaris changes:

  • Updated the AppProvider component
    • Breaking: Removed features class name toggle from setRootAttributes
    • Added a new theme prop which accepts a Polaris tokens ThemeName. This prop is used for:
      • Toggling the theme class names on the root element, and
      • Setting the theme object on the root ThemeContext.Provider
  • Added global theme toggle to Storybook

Polaris tokens changes:

  • Breaking: Moved the light-uplift overrides to base token groups (e.g. light-uplift is now the default theme)
  • Normalized specificity of the default theme

Note

Currently, our browser support requirements don't allow us to use the :where() selector to flatten our theme class name specificity to 0. Therefore, I'm proposing a solution that normalizes the specificity to 11.

Theme selector breakdown:

  • :root, html.p-<default-variant-theme>{...} - The generated stylesheet outputs the default theme custom properties to both the :root selector and a dedicated html.p-* selector. This allows the page to apply the default theme via :root initially (specificity 10) and then html.p-* when the AppProvider initializes (specificity 11)
  • html.p-<variant-theme-1>{...}html.p-<variant-theme-2> {...}html.p-... - All other variant themes have specificity 11
  • Thus, theme classes in the Admin should always have specificity 11 after rendering the AppProvider

Example theme selector output:

Screenshot 2023-09-12 at 3 12 07 PM

  • Initialized a new light-high-contrast-experimental theme
Screen.Recording.2023-09-12.at.5.01.17.PM.mov

@aaronccasanova aaronccasanova marked this pull request as ready for review September 13, 2023 00:07
@laurkim
Copy link
Contributor

laurkim commented Sep 13, 2023

Looking at the 300+ pending baselines that need to be accepted, I think a majority of them are coming from the fact that the font size is using the --p-font-size-100 token since we're no longer using the se23 flag class. I think this line in AppProvider.scss just needs to be updated to use the --p-font-size-80-experimental token that we used to apply behind the flag here.

Edit—
I pushed up a commit to fix the font styles in AppProvider and cleaned up some duplicate styling. Also pushed up a commit to consolidate some straggling se23 styles in IndexTable that was missed and causing some chromatic diff.

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

Nice! 🌟 🚀

Copy link
Member

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

Very nice work! What a contrast of a difference ☯️ 💯

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢

@aaronccasanova aaronccasanova merged commit 73d0fbe into next Sep 20, 2023
9 checks passed
@aaronccasanova aaronccasanova deleted the next-multi-theme branch September 20, 2023 22:15
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
Continuation of Shopify#10290

This PR contains breaking changes in both Polaris and Polaris tokens to
enable global multi-theme support.

### Polaris changes:
- Updated the `AppProvider` component
- **Breaking:** Removed `features` class name toggle from
`setRootAttributes`
- Added a new `theme` prop which accepts a Polaris tokens `ThemeName`.
This prop is used for:
    - Toggling the theme class names on the root element, and
    - Setting the theme object on the root `ThemeContext.Provider`
- Added global theme toggle to Storybook

### Polaris tokens changes:
- **Breaking:** Moved the `light-uplift` overrides to base token groups
(e.g. `light-uplift` is now the default theme)
- Normalized specificity of the default theme

> [!NOTE]
> Currently, our browser support requirements don't allow us to use the
`:where()` selector to flatten our theme class name specificity to `0`.
Therefore, I'm proposing a solution that normalizes the specificity to
`11`.

**Theme selector breakdown:**
- `:root, html.p-<default-variant-theme>{...}` - The generated
stylesheet outputs the default theme custom properties to both the
`:root` selector and a dedicated `html.p-*` selector. This allows the
page to apply the default theme via `:root` initially (specificity `10`)
and then `html.p-*` when the `AppProvider` initializes (specificity
`11`)
- `html.p-<variant-theme-1>{...}html.p-<variant-theme-2>
{...}html.p-...` - All other variant themes have specificity `11`
- Thus, theme classes in the Admin should always have specificity `11`
after rendering the `AppProvider`

**Example** theme selector output:

![Screenshot 2023-09-12 at 3 12 07
PM](https://github.com/Shopify/polaris/assets/32409546/88166226-512c-4cd5-a1d0-b77305929d5b)

- Initialized a new `light-high-contrast-experimental` theme


https://github.com/Shopify/polaris/assets/32409546/ad228532-e277-4107-af5c-95511f3e3297

---------

Co-authored-by: Lo Kim <lo.kim@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants