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

Deprecate JS theme in favor of CSS custom properties #2158

Merged
merged 9 commits into from Jun 25, 2023

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented Jun 17, 2023

Relates to #2153.

Purpose

Currently, the design tokens are distributed as a JavaScript object that can be used with CSS-in-JS libraries such as Emotion.js. We want to migrate away from CSS-in-JS to improve performance and compatibility with more frameworks.

CSS custom properties (aka CSS variables) are a natural choice. They're widely supported in modern browsers, can be used with any framework, and are fast to update.

Circuit UI began its migration to CSS custom properties with the introduction of the semantic color tokens (#1880). This pull request converts the remaining theme properties. Unlike the color tokens, the remaining tokens keep their familiar names and structure:

-${theme.borderRadius.circle}
+var(--cui-border-radius.circle)

The theme.breakpoints.* and theme.mq.* properties cannot be migrated because CSS custom properties aren't supported in media queries.

The theme.grid.* properties haven't been migrated because the grid components are legacy.

Approach and changes

  • Add the remaining properties to the design token schema and export all design tokens as CSS custom properties as @sumup/design-tokens/light.css.
  • Revert @sumup/design-tokens to CJS. The ESLint and Stylelint plugins, which depend on the package, don't support ESM yet.
  • Add the prefer-custom-properties ESLint rule to flag and automatically rewrite uses of the Emotion.js theme to CSS custom properties.
  • Update the theme documentation in Storybook with a consistent and informative table structure.
  • Mark the JavaScript theme, themePropType, and Theme interface as deprecated in @sumup/design-tokens.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@vercel
Copy link

vercel bot commented Jun 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 19, 2023 7:02pm

@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2023

🦋 Changeset detected

Latest commit: fa9b751

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@sumup/design-tokens Minor
@sumup/eslint-plugin-circuit-ui Minor
@sumup/circuit-ui Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@connor-baer connor-baer changed the title Deprecate Emotion.js theme in favor of CSS custom properties Deprecate JS theme in favor of CSS custom properties Jun 17, 2023
@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #2158 (fa9b751) into next (d4ab278) will increase coverage by 0.15%.
The diff coverage is 99.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #2158      +/-   ##
==========================================
+ Coverage   96.86%   97.02%   +0.15%     
==========================================
  Files         260      263       +3     
  Lines       23682    25063    +1381     
  Branches     2213     2265      +52     
==========================================
+ Hits        22940    24317    +1377     
- Misses        734      738       +4     
  Partials        8        8              
Impacted Files Coverage Δ
packages/design-tokens/scripts/build.ts 96.07% <96.07%> (ø)
packages/design-tokens/index.ts 97.05% <100.00%> (+0.76%) ⬆️
packages/design-tokens/themes/legacy/light.ts 100.00% <100.00%> (ø)
packages/design-tokens/themes/legacy/shared.ts 100.00% <100.00%> (ø)
packages/design-tokens/themes/light.ts 100.00% <100.00%> (ø)
packages/design-tokens/themes/schema.ts 100.00% <100.00%> (ø)
packages/design-tokens/utils/theme-prop-type.ts 100.00% <100.00%> (ø)
...n-circuit-ui/no-invalid-custom-properties/index.ts 100.00% <100.00%> (ø)
...lugin-circuit-ui/prefer-custom-properties/index.ts 100.00% <100.00%> (ø)

@connor-baer connor-baer marked this pull request as ready for review June 19, 2023 06:51
@connor-baer connor-baer requested a review from a team as a code owner June 19, 2023 06:51
@connor-baer connor-baer requested review from pdrmdrs and removed request for a team June 19, 2023 06:51
Copy link
Member

@nicosommi nicosommi left a comment

Choose a reason for hiding this comment

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

No vote here, but I left some comments to understand better the whole thing

.storybook/components/Theme.tsx Show resolved Hide resolved
packages/design-tokens/.gitignore Show resolved Hide resolved

```tsx
// _app.tsx
import '@sumup/design-tokens/light.css';
Copy link
Member

Choose a reason for hiding this comment

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

For switching between light and dark, how will that work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure yet. There's still a lot of uncertainty around the dark theme (Will it follow the system settings? Will users be able to set their preferred one? Will this be built into @sumup/design-tokens or inside each app?), so I consciously left this out of scope. We may need to adjust the API in the future, but that would be a small breaking change.

@connor-baer connor-baer merged commit 415c73d into next Jun 25, 2023
12 checks passed
@connor-baer connor-baer deleted the feature/custom-properties branch June 25, 2023 12:16
This was referenced Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants