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

25 button component #32

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

25 button component #32

wants to merge 8 commits into from

Conversation

nhistory
Copy link

No description provided.

@nhistory nhistory linked an issue Jan 19, 2025 that may be closed by this pull request
Comment on lines 130 to 143
"outline-gradient": {
"--gradient-color": "linear-gradient(135deg, #24eaca, #846de9)",
background: "transparent",
color: "text.outline",
border: "4px solid transparent",
backgroundClip: "padding-box, border-box",
backgroundOrigin: "padding-box, border-box",
borderImage: "var(--gradient-color)",
borderImageSlice: "1",
borderImageOutset: "0",
"&:active, &:hover": {
outline: "0",
},
},
Copy link
Author

Choose a reason for hiding this comment

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

image

@DaleSeo 달레스터디에서와 같은 방식으로 적용했는데 borderRadius 적용이 계속 안되네요.

Copy link
Author

Choose a reason for hiding this comment

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

@DaleSeo 버튼 컴포넌트의 varient에 필요한 요소들을 src/tokens/colors.ts 에 추가하는 방식으로 해서 작업했는데요. 이렇게 작업하는게 맞는지 궁금합니다. 일단 지금까지 작업이 맞다면 관련해서 테스트 코드도 작성해 볼려고 합니다. 생각보다 시간이 굉장히 많이 걸리네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

달레스터디에서와 같은 방식으로 적용했는데 borderRadius 적용이 계속 안되네요.

border-radius 속성은 적용되고 있는 것 같은데요? borderImage를 사용할 때 엣지 케이스 같은 게 있는 게 아닐까요? 판다가 예상하시는데로 각각의 속성을 모두 적용해주었는지 확인해보시면 어덜까요?

Shot.2025-01-19.at.18.35.11.mp4

Copy link
Contributor

@DaleSeo DaleSeo Jan 19, 2025

Choose a reason for hiding this comment

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

버튼 컴포넌트의 varient에 필요한 요소들을 src/tokens/colors.ts 에 추가하는 방식으로 해서 작업했는데요. 이렇게 작업하는게 맞는지 궁금합니다. 일단 지금까지 작업이 맞다면 관련해서 테스트 코드도 작성해 볼려고 합니다. 생각보다 시간이 굉장히 많이 걸리네요.

컴포넌트의 모든 varient에 대해서 디자인 토큰을 추가하시면 개발 생산성도 떨어지고 나중에 디자인 토큰 유지보수하기가 굉장히 힘들어질 겁니다. 여러 컴포넌트에 걸쳐서 재사용될 가능성이 높지 않은 varient에 대해서는 컴포넌트 내부에서 맵핑하시는 것을 추천드립니다.

level: {
1: { textStyle: "4xl" },
2: { textStyle: "3xl" },
3: { textStyle: "2xl" },
4: { textStyle: "xl" },
5: { textStyle: "lg" },
6: { textStyle: "md" },
},

Copy link
Author

Choose a reason for hiding this comment

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

버튼 컴포넌트의 varient에 필요한 요소들을 src/tokens/colors.ts 에 추가하는 방식으로 해서 작업했는데요. 이렇게 작업하는게 맞는지 궁금합니다. 일단 지금까지 작업이 맞다면 관련해서 테스트 코드도 작성해 볼려고 합니다. 생각보다 시간이 굉장히 많이 걸리네요.

컴포넌트의 모든 varient에 대해서 디자인 토큰을 추가하시면 개발 생산성도 떨어지고 나중에 디자인 토큰 유지보수하기가 굉장히 힘들어질 겁니다. 여러 컴포넌트에 걸쳐서 재사용될 가능성이 높지 않은 varient에 대해서는 컴포넌트 내부에서 맵핑하시는 것을 추천드립니다.

level: {
1: { textStyle: "4xl" },
2: { textStyle: "3xl" },
3: { textStyle: "2xl" },
4: { textStyle: "xl" },
5: { textStyle: "lg" },
6: { textStyle: "md" },
},

'컴포넌트 내부에서 맵핑'하는 것이 아래와 같이 하는 걸 의미하시는 걸까요?

solid: {
        background: { base: "{colors.violet.10}", _dark: "{colors.violet.1}" },
        color: { base: "{colors.violet.1}", _dark: "{colors.violet.10}" },
        "&:active, &:hover": {
          background: { base: "{colors.violet.8}", _dark: "{colors.violet.3}" },
          outline: "0",
        },
      },

Copy link
Author

Choose a reason for hiding this comment

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

달레스터디에서와 같은 방식으로 적용했는데 borderRadius 적용이 계속 안되네요.

border-radius 속성은 적용되고 있는 것 같은데요? borderImage를 사용할 때 엣지 케이스 같은 게 있는 게 아닐까요? 판다가 예상하시는데로 각각의 속성을 모두 적용해주었는지 확인해보시면 어덜까요?

Shot.2025-01-19.at.18.35.11.mp4

button 클래스 안에 bdr_10px이 들어가 있는 걸로 봐서 적용은 되어 있는 것 같은데 실제 border에는 굴곡이 들어가지 않는 상황입니다. borderRadius: "10px !important"로 해봐도 변화가 없는 걸 보니 뭔가 충돌이 있는건가 싶기도 하네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

'컴포넌트 내부에서 맵핑'하는 것이 아래와 같이 하는 걸 의미하시는 걸까요?

아니요. 색상 팔레트에 직접 접근하시기 보다는 시멘틱 토큰을 쓰셔야 할 것 같아요. 그러면 굳이 base_dark를 구분지어 명시할 일이 없어서 효율적입니다.

제가 아래 텍스트 컴포넌트에 맵핑한 코드를 보시면 좀 더 이해하시는 도움이 될 것 같습니다.

const styles = cva({
compoundVariants: [
{
muted: false,
tone: "neutral",
css: { color: "text" },
},
{
muted: false,
tone: "accent",
css: { color: "text.accent" },
},
{
muted: false,
tone: "danger",
css: { color: "text.danger" },
},
{
muted: false,
tone: "warning",
css: { color: "text.warning" },
},
{
muted: true,
tone: "neutral",
css: { color: "text.muted" },
},
{
muted: true,
tone: "accent",
css: { color: "text.muted.accent" },
},
{
muted: true,
tone: "danger",
css: { color: "text.muted.danger" },
},
{
muted: true,
tone: "warning",
css: { color: "text.muted.warning" },
},
],
});

Copy link
Contributor

@DaleSeo DaleSeo Jan 21, 2025

Choose a reason for hiding this comment

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

button 클래스 안에 bdr_10px이 들어가 있는 걸로 봐서 적용은 되어 있는 것 같은데 실제 border에는 굴곡이 들어가지 않는 상황입니다. borderRadius: "10px !important"로 해봐도 변화가 없는 걸 보니 뭔가 충돌이 있는건가 싶기도 하네요.

저도 원인지 뭔지 잘 모르겠네요... 😓 우리 여기에 너무 몰두하지 말고, gradient 지원은 별도의 티겟으로 빼서 나중에 구현하면 어떨까요?


export interface ButtonProps {
/** 버튼 텍스트 */
children: React.ReactNode;
type?: "button" | "submit";
onClick?: () => void;
variant?: ButtonVariant;
Copy link
Contributor

Choose a reason for hiding this comment

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

단일 속성으로 너무 다양한 종류의 버튼을 �표현하려고 하면 나중에 속성값의 개수가 폭발적으로 증가할 수 있으며 속성값의 이름을 짓는 것도 골치거리가 될 수 있습니다. 다음과 같이 두 개의 속성으로 분리하는 것을 제안드리고 싶습니다.

variant: "outline" | "solid" | "transparent";
tone?: "netural" | "accent" | "danger" | "warning" | "gradient";

Comment on lines 130 to 143
"outline-gradient": {
"--gradient-color": "linear-gradient(135deg, #24eaca, #846de9)",
background: "transparent",
color: "text.outline",
border: "4px solid transparent",
backgroundClip: "padding-box, border-box",
backgroundOrigin: "padding-box, border-box",
borderImage: "var(--gradient-color)",
borderImageSlice: "1",
borderImageOutset: "0",
"&:active, &:hover": {
outline: "0",
},
},
Copy link
Contributor

@DaleSeo DaleSeo Jan 21, 2025

Choose a reason for hiding this comment

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

button 클래스 안에 bdr_10px이 들어가 있는 걸로 봐서 적용은 되어 있는 것 같은데 실제 border에는 굴곡이 들어가지 않는 상황입니다. borderRadius: "10px !important"로 해봐도 변화가 없는 걸 보니 뭔가 충돌이 있는건가 싶기도 하네요.

저도 원인지 뭔지 잘 모르겠네요... 😓 우리 여기에 너무 몰두하지 말고, gradient 지원은 별도의 티겟으로 빼서 나중에 구현하면 어떨까요?

@@ -14,37 +28,17 @@ export const Button = ({
children,
type = "button",
onClick,
variant = "default",
style, // destructure the style prop
Copy link
Contributor

Choose a reason for hiding this comment

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

요거는 자치샇면 일관성을 해칠 수 있는 독이 될 수도 있을 것 같은데, 우선은 제외하시는 게 어떠실까요?

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.

Button Component
2 participants