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

[Feature] Button 컴포넌트 구현 #62

Merged
merged 27 commits into from
Jul 1, 2024
Merged

Conversation

hamo-o
Copy link
Member

@hamo-o hamo-o commented Jun 20, 2024

🎉 변경 사항

  • Button, TextButton 컴포넌트를 구현했습니다.

  • Button TextButton LinkButton에서의 공통 로직을 useButton hook으로 분리했습니다.

  • 피그마의 shadows 토큰 등록했습니다.

    image
  • disabled일 때나, pressed일 때 hover 상태의 스타일이 적용되는 문제가 있어 panda config에 아래 내용 추가했습니다.

    conditions: {
        hover: "&[aria-pressed=false]:not(:disabled):hover",
    },
    

🚩 관련 이슈

🙏 여기는 꼭 봐주세요!

  • codegen에서 오류가 발생하여 codegen/package.json에 아래 내용 추가했습니다. 문제가 된다면 말씀주세요!
image
  "name": "codegen",
  "private": true,
  // 이 부분 추가
  "type": "module",
  "devDependencies": {
    "plop": "^4.0.1"
  }
  • LinkButton은 더 알아보고 추가적으로 구현하겠습니다!

Copy link
Contributor

Copy link
Contributor

Copy link
Collaborator

@eugene028 eugene028 left a comment

Choose a reason for hiding this comment

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

섀도우 토큰까지 추가해주셔서 감사합니당 🥰
LinkButton은 제 생각을 첨언해보자면...
HTML에서 제공하고 있는 <a> 태그의 속성을 가진 버튼을 만들면 React에서 호환 가능하지 않을까 싶어요! (물론 Next.js에서 제공하는 <Link> 컴포넌트와 달라지겠지만요)

Next.js에서는 저희의 디자인 시스템에서 제공하는 <Link> 태그를 사용하기보다는 그쪽에서 제공하는 <Link> 내장API를 그대로 쓰는쪽이 어떨까 싶습니다!
디자인 시스템에서 제공하는 것을 1차원적인 기능이면 충분하지 않을까 싶어요!
리뷰 남긴 부분만 봐주세용~ 수고하셨습니다!

* @param {ComponentPropsWithRef<T>["ref"]} ref 렌더링된 요소 또는 컴포넌트에 연결할 ref.
*/

export interface CustomButtonProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P5;
CustomButtonProps에 요 type extend 해보는 것도 어떠실지..살짝..놓고갑니닷...ㅎㅎ
ButtonProps 타입 바로가기

packages/wow-ui/src/hooks/useButton.ts Outdated Show resolved Hide resolved
packages/wow-ui/src/components/Button/index.tsx Outdated Show resolved Hide resolved
packages/wow-ui/src/components/TextButton/index.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿 수고하셨습니다!
LinkButton은 기본으로는 a 태그를 사용하되 polymorphic하게 구현해서 Link 컴포넌트로 변경할 수 있게 제공하는 건 어떨까요?

packages/wow-ui/src/components/Button/index.tsx Outdated Show resolved Hide resolved
packages/wow-ui/src/components/Button/index.tsx Outdated Show resolved Hide resolved
packages/wow-ui/src/components/TextButton/index.tsx Outdated Show resolved Hide resolved
@hamo-o hamo-o force-pushed the feature/button-component branch from ccfe40d to d9f371a Compare June 30, 2024 09:16
Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

@hamo-o
Copy link
Member Author

hamo-o commented Jun 30, 2024

섀도우 토큰까지 추가해주셔서 감사합니당 🥰 LinkButton은 제 생각을 첨언해보자면... HTML에서 제공하고 있는 <a> 태그의 속성을 가진 버튼을 만들면 React에서 호환 가능하지 않을까 싶어요! (물론 Next.js에서 제공하는 <Link> 컴포넌트와 달라지겠지만요)

Next.js에서는 저희의 디자인 시스템에서 제공하는 <Link> 태그를 사용하기보다는 그쪽에서 제공하는 <Link> 내장API를 그대로 쓰는쪽이 어떨까 싶습니다! 디자인 시스템에서 제공하는 것을 1차원적인 기능이면 충분하지 않을까 싶어요! 리뷰 남긴 부분만 봐주세용~ 수고하셨습니다!

굿굿 수고하셨습니다! LinkButton은 기본으로는 a 태그를 사용하되 polymorphic하게 구현해서 Link 컴포넌트로 변경할 수 있게 제공하는 건 어떨까요?

react-router-domLink 컴포넌트, next/linkLink 컴포넌트 등이 있는데, 이를 도입하기 위해서는 현재 ComponentPropsWithoutRef 등으로 기본 html 속성의 타입을 지정하는 방법은 사용하지 못할 것 같습니다. 우선은 유진님 말씀대로 각 서비스에서 제공하는 기능들을 사용하여 중첩된 구조를 사용하고, 서현님께서 언급하신 부분은 온보딩 2차 마무리 후 구현하고 그때 바꾸면 어떨까요!!

Copy link
Collaborator

@SeieunYoo SeieunYoo left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다 🤗

저도 디자인 시스템의 컴포넌트는 퓨어한 기능과 디자인을 가진 컴포넌트 여야 한다고 생각합니당
그래서 Next 환경이나 react-router-dom 의 Link 컴포넌트의 기능을 받을 수 있도록 고려해야 할까,,?! 라는 생각이 들어유 말씀대로 사용하는 쪽에서 Link 컴포넌트의 children 으로 버튼을 넣어서 구현하는 방향이 맞지 않을까 생각합니다!

packages/wow-ui/src/components/TextButton/index.tsx Outdated Show resolved Hide resolved
Comment on lines +33 to +41
disabled?: boolean;
size?: "lg" | "sm";
onKeyUp?: () => void;
onKeyDown?: () => void;
onMouseLeave?: () => void;
onPointerDown?: () => void;
onPointerUp?: () => void;
style?: CSSProperties;
className?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
Button 의 props 와 겹치는 부분이 많은 것 같은데 공통 props 로 겹치는 부분들을 빼는 건 어떨까요?!

Comment on lines 88 to 107
<Component
aria-disabled={disabled}
aria-pressed={pressed}
className={TextButtonStyle}
disabled={disabled}
ref={ref}
onKeyDown={handleKeyDown}
onKeyUp={handleKeyUp}
onMouseLeave={handleMouseLeave}
onPointerDown={handlePointerDown}
onPointerUp={handlePointerUp}
{...rest}
>
<styled.span
textDecoration="underline"
textStyle={size === "lg" ? "label1" : "label2"}
>
{label}
</styled.span>
</Component>
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
Button 과 TextButton 의 공통된 로직이 많은 것 같은데 공통된 부분을 모아서 Component 영역을 BaseButton 같은 컴포넌트로 추상화 시키면 어떨까욥?!

 <BaseButton className={TextButtonStyle} {...rest}>
    <styled.span
      textDecoration="underline"
      textStyle={size === "lg" ? "label1" : "label2"}
    >
      {label}
    </styled.span>
  </BaseButton>

Copy link
Member Author

Choose a reason for hiding this comment

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

요거 그러면 common 이라든지 다른 폴더에서 공통 컴포넌트 관리하려고 하는데 어떻게 생각하시나유,,
얘도 같이 빌드해서 배포하면 혼동이 있을 것 같아서요. 쌓이게 되면 base끼리 따로 모아서 배포하는것도 좋을 것 같아요

Copy link
Collaborator

@SeieunYoo SeieunYoo Jul 1, 2024

Choose a reason for hiding this comment

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

아항 좋아요 components 외부에 폴더를 두면 좋을 것 같네욥
나중에 빌드에 포함시키고 싶지 않지만 개발 상에서 쓰이는 컴포넌트 파일이 필요한 경우가 생길 것 같은데 어떻게 빌드에 포함시키지 않을 지 고민해보아야 겠네용,,🤔
(스크립트에서 제외시키고 싶은 파일 제목에 대한 배열을 만들어서 제외를 하는 방법 등등,,)

Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿

Copy link
Contributor

github-actions bot commented Jul 1, 2024

Copy link
Contributor

github-actions bot commented Jul 1, 2024

@hamo-o
Copy link
Member Author

hamo-o commented Jul 1, 2024

아래 이슈에서 나머지 리뷰들 반영하겠습니다!

@hamo-o hamo-o merged commit 4d7115a into main Jul 1, 2024
3 checks passed
@hamo-o hamo-o deleted the feature/button-component branch July 1, 2024 18:41
@hamo-o hamo-o linked an issue Jul 1, 2024 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] 버튼 컴포넌트 구현
4 participants