refactor(Button): Button 컴포넌트 variant 추가 #287

imddoy · 2025-09-30T13:46:27Z

변경사항

링크

시급한 정도

🏃‍♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.

기타 사항

기존variant 속성을 유지하기 위해 intent 속성 추가

height · 2025-09-30T13:46:31Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

changeset-bot · 2025-09-30T13:46:32Z

⚠️ No Changeset found

Latest commit: 55937c7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

constantly-dev

수고하셨습니다! 코멘트 확인해주세요~

constantly-dev · 2025-12-27T04:28:29Z

packages/ui/Button/Button.tsx

-  const style = createButtonVariant(theme, rounded, size, variant);
+  const { finalIntent, finalShape } = useResolvedProps({ intent, shape, theme, rounded });
+  const isFloating = variant === 'floating';
+  const scrollDirection = isFloating ? useScrollDirection() : null;


hook은 조건에 따라 호출되는 것이 아닌 최상위에서 호출이 되어야 할 것 같아요! 차라리 일단 무조건 값을 return 받고 값으로 분기처리 해야 할 것 같아요.

const scrollDirection = useScrollDirection(); const finalDirection = isFloating ? scrollDirection : null;

근데 이렇게 하면 useScrollDirection에서 useEffect 내부 이벤트 리스너 등 로직을 isFloating이 false일 때도 불필요하게 실행이 되니, useScrollDirection에 enabled 파라미터를 뚫어서 isFloating에 따라 useEffect에 early return으로 실행 분기를 내부적으로 추가하면 어떨까요?

가장 최선은 디자이너와 논의하여 FloatingButton 정도의 별도 컴포넌트를 만드는 것이라고 봐요.. 하나의 베리언트가 이렇게 많은 파생 분기 코드들을 만들어내니, 자연스럽게 분리를 고려하기 좋은 타이밍이라고 생각합니다.

그게 안된다면 차선책으로 진혁님이 말씀해주신 것들 챙기면 좋을 것 같네요

저도 이부분에 대해서 동의합니다!

constantly-dev · 2025-12-27T04:31:06Z

packages/ui/Button/hooks.ts

+}
+
+export const useResolvedProps = ({ intent, shape, theme, rounded }: UseResolvedProps) => {
+  const finalIntent = React.useMemo(() => {


useMemo만 import 시켜도 될 것 같습니다

constantly-dev · 2025-12-27T04:40:41Z

packages/ui/Button/hooks.ts

+  rounded?: 'md' | 'lg';
+}
+
+export const useResolvedProps = ({ intent, shape, theme, rounded }: UseResolvedProps) => {


button 컴포넌트 interface가 변경되면서 useResolvedProps로 이전 interface의 호환성을 가져가기 위함이 맞을까요?
물론 개인적인 생각이지만 호환성을 위해서는 필요하지만 이렇게 진행하게 되면 결국 레거시가 또 추가되는 느낌이 들어서..! 이후에 각 팀에서 interface 최신화해주고, 없애는 것은 어떨까요?

constantly-dev · 2025-12-27T05:01:27Z

packages/ui/Button/hooks.ts

+  useEffect(() => {
+    let lastScrollY = window.pageYOffset;
+    const updateScrollDirection = () => {
+      const scrollY = window.pageYOffset;
+      const direction = scrollY > lastScrollY ? 'down' : 'up';
+      setScrollDirection(direction);
+      lastScrollY = scrollY > 0 ? scrollY : 0;
+    };
+    window.addEventListener('scroll', updateScrollDirection);
+    return () => {
+      window.removeEventListener('scroll', updateScrollDirection);
+    };
+  }, [scrollDirection]);


현재 코드상으로는 scrollDirection가 의존에 추가되어 있어 스크롤 할 때마다 useEffect 전체 로직이 실행되니까, 이벤트 리스너 로직도 계속 실행이 될 것 같아요.
scrollDirection가 의존성 배열에 안 들어가도 될 것 같은데 어떻게 생각하시나요?

constantly-dev · 2025-12-27T05:06:01Z

packages/ui/Button/hooks.ts

+  const [scrollDirection, setScrollDirection] = useState<'up' | 'down' | null>(null);
+
+  useEffect(() => {
+    let lastScrollY = window.pageYOffset;


클로저로 구현된 거 같은데 맞을까요?? 지금도 잘 작동할 것 같고 useRef를 사용해도 동일하게 작동하겠네요!

constantly-dev · 2025-12-27T05:12:39Z

packages/ui/Button/utils.ts

  sizeTheme: ButtonSizeTheme,
-  variant: 'fill' | 'outlined',
+  variant: ButtonVariant,
+  isExpanded: boolean,


피그마랑 동일하게 isExtended로 통일하는 것은 어떨까요?

Suggested change

isExpanded: boolean,

isExtended: boolean,

아래 분기 처리된 string 값도 extended 같아서!

padding: variant === 'floating' ? isExpanded ? 'floating-extended' : 'floating-default' : variant === 'text' ? 'text' : sizeTheme,

constantly-dev · 2025-12-27T05:13:59Z

packages/ui/Button/Button.tsx

+  const { finalIntent, finalShape } = useResolvedProps({ intent, shape, theme, rounded });
+  const isFloating = variant === 'floating';
+  const scrollDirection = isFloating ? useScrollDirection() : null;
+  const [isExpanded, setIsExpanded] = useState(false);


isExtended로 피그마와 통일하는 것은 어떨까요?

constantly-dev · 2025-12-27T05:14:32Z

packages/ui/Button/Button.tsx

  RightIcon,
  variant = 'fill',
+  shape = 'rect',
+  intent = 'primary',


단순 궁금증인데 intent는 어떤 의미인가요?

constantly-dev

👀

constantly-dev · 2025-12-27T05:20:00Z

packages/ui/Button/utils.ts

+    padding:
+      variant === 'floating'
+        ? isExpanded
+          ? 'floating-extended'
+          : 'floating-default'
+        : variant === 'text'
+          ? 'text'
+          : sizeTheme,


Do not nest ternary expressions. eslint(no-nested-ternary)

eslint error가 뜨긴하네요!

constantly-dev · 2025-12-27T05:20:39Z

packages/ui/Button/utils.ts

    },
-    borderRadius: radiusTheme === 'lg' ? 'max' : sizeTheme,
-    padding: sizeTheme,
+    borderRadius: variant === 'text' || variant === 'floating' ? variant : radiusTheme === 'pill' ? 'max' : sizeTheme,


조금 복잡한 조건은 차라리 이름 붙여서 분리해주면 좋을 것 같아요!

wuzoo · 2025-12-27T21:38:14Z

packages/ui/Button/hooks.ts

+interface UseResolvedProps {
+  intent: ButtonIntent;
+  shape: ButtonShape;
+  theme?: ButtonColorTheme;
+  rounded?: 'md' | 'lg';
+}


Suggested change

interface UseResolvedProps {

intent: ButtonIntent;

shape: ButtonShape;

theme?: ButtonColorTheme;

rounded?: 'md' | 'lg';

}

type UseResolvedProps = Pick<ButtonProps, pick_union>

새로 작성하지 않는게 좋을 것 같아요

wuzoo · 2025-12-27T22:01:53Z

packages/ui/Button/Button.tsx

-  const style = createButtonVariant(theme, rounded, size, variant);
+  const { finalIntent, finalShape } = useResolvedProps({ intent, shape, theme, rounded });
+  const isFloating = variant === 'floating';
+  const scrollDirection = isFloating ? useScrollDirection() : null;


가장 최선은 디자이너와 논의하여 FloatingButton 정도의 별도 컴포넌트를 만드는 것이라고 봐요.. 하나의 베리언트가 이렇게 많은 파생 분기 코드들을 만들어내니, 자연스럽게 분리를 고려하기 좋은 타이밍이라고 생각합니다.

그게 안된다면 차선책으로 진혁님이 말씀해주신 것들 챙기면 좋을 것 같네요

imddoy added 5 commits September 30, 2025 15:08

refactor: 버튼 theme, rounded 속성을 다자인 명세에 맞게 intent, shape을 사용하도록 변경

76dbf95

기존variant 속성을 유지하기 위해 intent 속성 추가

chore: 주석 type -> intent 오탈자 수정

7b5466b

feat: 텍스트 버튼 추가

c640708

feat: 플로팅 버튼 추가

2bd93dd

chore: 스토리북 show code 컴포넌트 태그명 지정

55937c7

pull-request-size bot added the size/L label Sep 30, 2025

imddoy self-assigned this Sep 30, 2025

constantly-dev reviewed Dec 27, 2025

View reviewed changes

wuzoo reviewed Dec 27, 2025

View reviewed changes

refactor(Button): Button 컴포넌트 variant 추가 #287

Are you sure you want to change the base?

refactor(Button): Button 컴포넌트 variant 추가 #287

Uh oh!

Conversation

imddoy commented Sep 30, 2025

변경사항

링크

시급한 정도

기타 사항

Uh oh!

height bot commented Sep 30, 2025

Uh oh!

changeset-bot bot commented Sep 30, 2025

⚠️ No Changeset found

Uh oh!

constantly-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constantly-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants