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

fix(styles,theme): add S2 tokens and theme #4241

Open
wants to merge 132 commits into
base: main
Choose a base branch
from
Open

Conversation

TarunAdobe
Copy link
Contributor

@TarunAdobe TarunAdobe commented Apr 3, 2024

Introduce spectrum-two styles as a SWC theme.

Description

Todos:

  • Add a script to generate tokens-v2 from the @spectrum-css's next versions.
  • Add spectrum-two theme that takes v2 tokens.
  • Update storybook decorator and documentation site to include spectrum-two theme.
  • Update visual ci test to include spectrum-two theme.
  • Think about documenting these changes

Motivation and context

How has this been tested?

  • Test case 1
    1. Go here
    2. Toggle the theme
  • Test case 2
    1. Go here
    2. Toggle the theme

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@TarunAdobe TarunAdobe self-assigned this Apr 3, 2024
@Rajdeepc
Copy link
Contributor

Rajdeepc commented Apr 3, 2024

Good start! Let's also think about creating some documentation around this in parallel!

Rajdeepc
Rajdeepc previously approved these changes May 3, 2024
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for resolving the VRT's

@Westbrook
Copy link
Collaborator

Great work in pulling out the lightest/darkest listing for S2.

Something is still coming up strangely in the VRT output:
image

After that, I think we're good 🤞🏼

@Rajdeepc
Copy link
Contributor

Rajdeepc commented May 3, 2024

Great work in pulling out the lightest/darkest listing for S2.

Something is still coming up strangely in the VRT output: image

After that, I think we're good 🤞🏼

@TarunAdobe In review.js this should be metadata.system instead of metadata.theme. This is why it is returning an empty side-nav-item
Screenshot 2024-05-03 at 5 58 59 PM

Rajdeepc
Rajdeepc previously approved these changes May 8, 2024
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests and also the dev mode warning for spectrum-two.

tools/theme/src/spectrum-two/theme.css Outdated Show resolved Hide resolved
@@ -110,6 +115,7 @@
"@spectrum-css/commons": "^9.1.3",
"@spectrum-css/expressvars": "^3.0.9",
"@spectrum-css/tokens": "^13.2.0",
"@spectrum-css/tokens-v2": "npm:@spectrum-css/tokens@^14.0.0-next.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is now a "stable" 14.0 release, should CSS get you a new "future" release with that info?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a import for spectrum-two. I think the stable 14.0 version is importing S1 tokens

tools/theme/test/theme-devmode.test.ts Outdated Show resolved Hide resolved
tools/theme/src/Theme.ts Show resolved Hide resolved
@Rajdeepc Rajdeepc force-pushed the swc-s2-styles branch 3 times, most recently from 26ec02c to 2c5c4a6 Compare May 13, 2024 10:38
@Rajdeepc Rajdeepc force-pushed the swc-s2-styles branch 4 times, most recently from 038816e to 33c5c24 Compare May 13, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants