-
Notifications
You must be signed in to change notification settings - Fork 47
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
🚀 feat: create new theme #723
Conversation
hopeNew, | ||
relaxNew, | ||
neon, | ||
sand, | ||
peaceNew, | ||
upliftNew, | ||
deepPurpleNew, | ||
successNew, | ||
neutralNew, | ||
staminaNew, | ||
deepNew, | ||
mediumNew, | ||
lightNew, | ||
lightest, | ||
clearNew, |
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.
potential issue: isn't this going to require changes in all the components that use the colours directly?
- They'd have to know whether to pick the
new
and the old. A few components already do it via avariant
prop IIRC. - When we inevitably remove the old one, won't we need to remove the
new
suffix, creating another potential breaking change?
(there's another discussion here... should the components read the colours directly?)
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.
We are solving this with the theme: https://github.com/gympass/yoga/pull/723/files#diff-7c44dab781eac6c7c111934646f7760210c5556222abaace43c08a350788f217R7-R18
Components access the values from the theme and not the tokens directly (although they are the same in this case). As you said, using the tokens directly is probably another discussion and I agree we shouldn't be doing this, but having it inside the theme helped us keep everything using the expected colors 😄.
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.
yeah! I was thinking of just having it done on the theme itself but not exposing it via the tokens. Do you think it's worth it to "hide" these new tokens on packages/yoga/src/Theme/theme/theme.js#theme
function?
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.
Maybe in the future, as it will be a breaking change.
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 meant only the new
suffixed ones 😅
packages/yoga/src/Theme/helpers/themeGenerator/themeGenerator.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,58 @@ | |||
import createTheme from '../helpers/themeGenerator'; | |||
|
|||
export const v3theme = createTheme(tokens => ({ |
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.
nitpick: I think we shouldn't call it v3 as the yoga package is already on 7 :P might be confusing for who is importing it. What about something like refreshedTheme
or newTheme
?
Description 📄
This PR creates and exports a new theme, based on new tokens. This new theme can be used on applications by passing the
theme
prop to theThemeProvider
.Platforms 📲
Type of change 🔍
How Has This Been Tested? 🧪
Find the places on the doc/labnative that are using the
ThemeProvider
and add the new theme to it. E.g.:Checklist: 🔍