-
Notifications
You must be signed in to change notification settings - Fork 646
Use CSS color vars with fallback in legacy Primitive tokens #4157
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
Conversation
🦋 Changeset detectedLatest commit: fbc0e26 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
| }, | ||
| expander: { | ||
| icon: '#7d8590', | ||
| icon: 'var(--diffBlob-expander-iconColor, var(--color-diff-blob-expander-icon, #848d97))', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that these changes from 7d8590 to 848d97 are actually intended and correct ✅
| inputPlaceholderText: '#6e7681', | ||
| inputFocusText: '#e6edf3', | ||
| inputBg: '#161b22', | ||
| inputShadow: '0 0 0 1px (obj) => (0, get_1.default)(obj, path)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been broken for awhile, I replaced it with the correct value
joshblack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🥳 I had two broader questions as it relates to this PR:
- Is there any potential impact as it relates to how folks use
themeGet? It seems like most people just use it to get a value versus interacting with it so seems like it should be good? - What's the best way for us to test this out in dotcom to make sure we're confident no styling issues make their way into a release?
Closes https://github.com/github/primer/issues/2987
As a followup to moving the legacy theme files into Primer React from Primer Primitives, this PR introduces the new v8 color CSS vars with fallbacks as the value of the JS variables. This allows us to staff ship v8 variables without rewriting all of our CSS upfront. It also may help with the theme flashing issue.
Code review
This looks like a lot of changes (it is) but the thing we are most concerned about is if the raw value has changes. The easiest way to see that is to primarily review the lefthand side to see which values have changed. There are a few intentional changes:
transparentChangelog
Rollout strategy
Testing & Reviewing
We'll definitely want to test this against dotcom with an integration test PR.
Merge checklist