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

Color tokens test page updates #3023

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

Conversation

lyzhan7
Copy link
Contributor

@lyzhan7 lyzhan7 commented Aug 10, 2023

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

  1. The color tokens test page was showing either 1) tokens from the win32 theme if the theme was a win32 theme or 2) tokens created using default theme code. For macOS, we were showing macOS tokens, but these tokens were going through default theme code rather than macOS theme code. This resulted in some expected tokens not showing up, and in some tokens showing up as undefined. Let's just show all the tokens in theme - this should at least make sense for every platform.
  2. I noticed that the swatch color wasn't updating when switching from light/dark mode. This was because when we were re-rendering after a light/dark mode switch, we would check first to see if a color with that same colorName was defined already (which it would be, but with the wrong light/dark mode). I updated this code so that we just update the color of the swatch every time we render - if there's a better way to do this though please let me know.

Notes:

  • After this change, at least on macOS, there are more tokens shown in the color token test page. This is because the tokens we expose through the theme contain 1) tokens from the pipeline/design-tokens repo, as well as 2) tokens we define in FURN, which we should remove at some point. Previous to this change we were only showing tokens from 1), but now we are showing both. All the 2) tokens should be removed at some point.
  • Some of the 2) tokens (defined in FURN) show up as [object Object] - either because they are DynamicColorMacOS or they are PlatformColor. Given that we should remove them at some point, I've left them as is.

Verification

Before:

before.mov

After:

after.mov

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@lyzhan7 lyzhan7 requested a review from a team as a code owner August 10, 2023 18:29
@Saadnajmi
Copy link
Collaborator

  • either because they are DynamicColorMacOS or they are PlatformColor. Given that we should remove them at some point, I've left them as is.

I don't think that's necessarily true? I thought a goal might be to have support for them in the token system the way windows/win32 do right now.

@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Aug 10, 2023

  • either because they are DynamicColorMacOS or they are PlatformColor. Given that we should remove them at some point, I've left them as is.

I don't think that's necessarily true? I thought a goal might be to have support for them in the token system the way windows/win32 do right now.

Not sure about PlatformColor - but pretty sure we want to remove DynamicColorMacOS and rely on the light/dark theme tokens instead

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.

2 participants