refactor: sync token provider and refactor theme provider#192
refactor: sync token provider and refactor theme provider#192froggy1014 merged 17 commits intorelease/v1from
Conversation
- remove darkmode
froggy1014
requested review from
dev-hobin,
haedawn,
jiji-hoon96,
leejiwonn,
noahluftyang and
synuns
as code owners
|
✅ Deploy Preview for side-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
packages/tokens/src/colors/colors.ts
Outdated
| export const themeColor = { | ||
| '1st': { | ||
| primary: '#01fe13', | ||
| secondary: '#01fe13', | ||
| background: '#000000', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #01fe13 0%, #000000 100%)', | ||
| }, | ||
| '2nd': { | ||
| primary: '#03ff31', | ||
| secondary: '#06ffe3', | ||
| background: '#131518', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #03ff31 0%, #06ffe3 100%)', | ||
| }, | ||
| '3rd': { | ||
| primary: '#00ffff', | ||
| secondary: '#00ff99', | ||
| background: '#0d0d0d', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #00FFFF 0%, #00FF99 100%)', | ||
| }, | ||
| '4th': { | ||
| primary: '#f4a1a0', | ||
| secondary: '#f4a1a0', | ||
| background: '#0f1010', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #FF9595 0%, #FFE5B1 100%)', | ||
| }, |
There was a problem hiding this comment.
컬러 토큰에서 각 기수별로 관리가 되어야하는지에 대해서 논의해보면 좋을 것 같아요.
기수별 토큰이 디자인 시스템에서 관리되어야 한다면 책임이 너무 커지는 것 같아요.
일단 책임을 따로 두고 default 값만 현재 기수의 컬러 토큰으로두고,
프로바이더를 통해서 외부에서 주입받는 식으로 관리되는게 어떨까요?
There was a problem hiding this comment.
신원이 말도 동의하는데, 기수별로 토큰 값을 사용할거면 오브젝트 형태로 관리된 요소가 있으면 좋겠다는 생각도 있어!
There was a problem hiding this comment.
현재 단계에서는 단순하게 몇개의 테마만 지원하는데, 외부에서 주입받는것은 다소 과하지않을까라는 생각도 들기도해.
초기상태에는 명확한 파악을 할때 현재 구조가 도움이 될 것같아.
하지만 개선을 한다면 default 객체를 정의하고 prop을 기수를 '1st'와 같이 문자로 받는 대신 기수와 관련된 인터페이스를 정의해서 객체를 주입받도록 하는건 어떨까?
interface ThemColor {
primary: string;
secondary: string;
....
}const defaultThemeColor: ThemColor = {
primary: '#01fe13',
secondary: '#01fe13'
....
} interface ThemeProviderProps {
theme?: ThemColor;
children: ReactNode;
}export const ThemeProvider = ({ theme = defaultThemeColor, children }: ThemeProviderProps) => {
...
}There was a problem hiding this comment.
@jiji-hoon96 @synuns @kimdaeyeobbb
피드백을 보고 생각을 해봤는데,
1st ~ 4th 의 테마 색상들은 객체형태로 토큰에 남겨놓고 default로는 현재기수(4th) 가져오게끔 변경해봤어요.
theme 색상 자체를 외부에서 받을 수 있도록 ThemeColor라는 인터페이스를 도입 바꿔봤습니다. @kimdaeyeobbb
어떻게 생각하시나요.. 괜찮으면 바로 머지할게요 일단.
export interface ThemeColor {
primary: string;
secondary: string;
background: string;
text: string;
gradient: string;
}이 과정에서 data-theme 통한 기존에 정의된 테마를 가져오는게 아닌,
동적으로 색상이 주입될 수 있도록 내부적으로 자바스크립트로 CSS 변수를 직접 바꿀거같아요.
Object.entries(theme).forEach(([key, value]) => {
element.style.setProperty(`--side-color-${key}`, value);
});// AS-IS
<ThemeProvider theme="4th" /> // 오직 4가지 테마만
// TO-BE
<ThemeProvider theme={themeColor['1st']} />
<ThemeProvider theme={{
primary: '#ff6b6b', // 원하는 색상
secondary: '#4ecdc4', // 자유롭게
background: '#1a1a2e', // 설정 가능
text: '#ffffff',
gradient: 'linear-gradient(45deg, #ff6b6b, #4ecdc4)'
}} />스토리북 참고해주세요.
|
이거 처리해야 후속작업들 토큰 사용하는걸로 리팩토링 할 수 있어서 token 작업은 머지해도 괜찮을 것 같아요 |
packages/tokens/src/colors/colors.ts
Outdated
| export const themeColor = { | ||
| '1st': { | ||
| primary: '#01fe13', | ||
| secondary: '#01fe13', | ||
| background: '#000000', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #01fe13 0%, #000000 100%)', | ||
| }, | ||
| '2nd': { | ||
| primary: '#03ff31', | ||
| secondary: '#06ffe3', | ||
| background: '#131518', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #03ff31 0%, #06ffe3 100%)', | ||
| }, | ||
| '3rd': { | ||
| primary: '#00ffff', | ||
| secondary: '#00ff99', | ||
| background: '#0d0d0d', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #00FFFF 0%, #00FF99 100%)', | ||
| }, | ||
| '4th': { | ||
| primary: '#f4a1a0', | ||
| secondary: '#f4a1a0', | ||
| background: '#0f1010', | ||
| text: color.white, | ||
| gradient: 'linear-gradient(45deg, #FF9595 0%, #FFE5B1 100%)', | ||
| }, |
There was a problem hiding this comment.
신원이 말도 동의하는데, 기수별로 토큰 값을 사용할거면 오브젝트 형태로 관리된 요소가 있으면 좋겠다는 생각도 있어!
- Changed ThemeProvider to accept ThemeColor objects instead of string literals for themes. - Updated related tests and stories to reflect the new theme structure. - Improved theme handling by applying CSS variables directly from the ThemeColor object.
4 participants
Changes
Token
피그마에서 정의된 Token에 대해서는 수정 및 추가했습니다.
Theme Provider
Button 예제
Visuals
Checklist
Additional Discussion Points