-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
detach from Figma's tokens #2140
Conversation
🦋 Changeset detectedLatest commit: bb5e9ff 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 |
Commit SHA:dd93d4dcbad1378d84ba22e65e72065bdce7c09a Test coverage results 🧪
|
/review |
PR Analysis
PR Feedback
How to use
|
/describe |
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.
Only a couple of minor comments - nothing major.
This looks really good - and will be super helpful for iterating designs!
'&:focus-visible': { | ||
boxShadow: '$focus', | ||
}, | ||
}, | ||
ghost: { | ||
background: 'transparent', | ||
padding: '$2 $3', | ||
color: '$fgBtnGhost', | ||
'&:hover': { |
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.
Do ghost buttons not have hover states?
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.
that must've been by mistake 👍
@@ -110,7 +110,7 @@ export const ThemeSelector: React.FC = () => { | |||
return ( | |||
filteredThemes.length > 0 && ( | |||
<DropdownMenuRadioGroup value={typeof activeTheme[groupName] !== 'undefined' ? activeTheme[groupName] : ''}> | |||
<Text css={{ color: '$staticTextMuted', padding: '$2 $3' }}>{groupName === INTERNAL_THEMES_NO_GROUP ? INTERNAL_THEMES_NO_GROUP_LABEL : groupName}</Text> | |||
<Text css={{ color: '$contextMenuForegroundMuted', padding: '$2 $3' }}>{groupName === INTERNAL_THEMES_NO_GROUP ? INTERNAL_THEMES_NO_GROUP_LABEL : groupName}</Text> |
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 token name doesn't match the naming convention of 'fg' for foreground
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.
💯 i will create a followup PR for a second refactor just to keep this PR a bit smaller. i'd also like to change textMuted / textDefault
over to fgDefault
etc
src/hooks/useFigmaTheme.ts
Outdated
@@ -5,8 +5,21 @@ export function useFigmaTheme() { | |||
|
|||
useEffect(() => { | |||
const htmlClassList = document.documentElement?.classList || []; | |||
console.log('useFigmaTheme', isDarkTheme, htmlClassList); |
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.
Do we need the console log here in production?
changes naming on a few tokens to align with Studio tokens, followup to #2140
This is a WIP PR to detach from Figma's tokens and instead ship our own.
/describe