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 IssueLabelToken treating alternative light schemes as dark #3104

Merged
merged 6 commits into from
Mar 31, 2023

Conversation

iansan5653
Copy link
Contributor

@iansan5653 iansan5653 commented Mar 31, 2023

The IssueLabelToken conditionally picks colors based on whether colorScheme === 'light'. This strategy assumes that there exist only two color schemes, when in reality there are many.

The fix for this is to use resolvedColorMode, which has four possible values: dark, night, light, and day (it's not clear what the difference between dark and night or light and day are, but this is what the value is typed as). So instead, to accurately determine whether the color scheme is a light one, we can see if resolvedColorMode is light or day.

Update: instead of using resolvedColorMode we decided to use resolvedColorTheme and see if it startsWith the expected color mode. This is less robust to new theme name formats but somewhat safer because there is no guarantee that the color mode actually matches the theme in real life, and the theme is what really determines the colors that are rendered.

Alternatively, we could instead call colorScheme.includes('light'), but I avoided this solution because it feels fragile (not robust to the possibility of future themes that are named differently.

Closes #3098

Before:

IssueLabelToken in all themes, looking correct in 'light' and in all dark themes but wrong in alternative light themes

After:

IssueLabelToken looking correct in all light and dark themes

@iansan5653 iansan5653 requested review from a team and mperrotti March 31, 2023 00:34
@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

🦋 Changeset detected

Latest commit: 6390d9f

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

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions github-actions bot temporarily deployed to storybook-preview-3104 March 31, 2023 00:42 Inactive
Copy link
Contributor

@colebemis colebemis 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 the quick fix ⚡

@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 95.09 KB (+0.03% 🔺)
dist/browser.umd.js 95.63 KB (+0.03% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-3104 March 31, 2023 00:43 Inactive
@iansan5653 iansan5653 temporarily deployed to github-pages March 31, 2023 00:46 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3104 March 31, 2023 00:46 Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Thanks! 🚀 Just missing a changeset.

@github-actions github-actions bot temporarily deployed to storybook-preview-3104 March 31, 2023 01:01 Inactive
@iansan5653 iansan5653 temporarily deployed to github-pages March 31, 2023 01:04 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3104 March 31, 2023 01:05 Inactive
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.

IssueLabelToken has extremely low contrast in high contrast color modes
3 participants