feat: add new FloatingBar component
#359
Conversation
🦋 Changeset detectedLatest commit: 5d425b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @noahchoii, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
새로운 ActionBar 컴포넌트 추가에 대한 PR을 리뷰했습니다. 전반적으로 컴포넌트 구조와 기능 구현이 잘 이루어졌습니다. 문서와 예제 코드도 충실하게 작성되었습니다. 몇 가지 개선점을 제안합니다. 예제 코드의 잠재적인 버그 수정, 스타일 가이드 준수를 위한 displayName 설정 및 상수 타입 강화, 그리고 문서의 명확성 개선에 대한 의견을 남겼습니다. 이 피드백들이 컴포넌트의 완성도를 높이는 데 도움이 되기를 바랍니다.
| const handleItemChange = (itemId: string, isChecked: boolean) => { | ||
| setSelectedItems((prev) => ({ ...prev, [itemId]: isChecked })); | ||
|
|
||
| const newSelectedCount = isChecked ? selectedCount + 1 : selectedCount - 1; | ||
|
|
||
| setOpen(newSelectedCount > 0); | ||
| }; |
There was a problem hiding this comment.
handleItemChange 함수에서 newSelectedCount를 계산할 때 이전 렌더링의 selectedCount 값을 사용하고 있어, 여러 상태 업데이트가 동시에 발생할 때 오래된 상태(stale state)를 참조하는 버그가 발생할 수 있습니다. setSelectedItems는 비동기적으로 동작하기 때문입니다.
안전하게 상태를 업데이트하려면 setSelectedItems의 함수형 업데이트를 사용하여 이전 상태를 기반으로 새 상태를 계산하고, 그 안에서 setOpen을 호출하는 것이 좋습니다.
| const handleItemChange = (itemId: string, isChecked: boolean) => { | |
| setSelectedItems((prev) => ({ ...prev, [itemId]: isChecked })); | |
| const newSelectedCount = isChecked ? selectedCount + 1 : selectedCount - 1; | |
| setOpen(newSelectedCount > 0); | |
| }; | |
| const handleItemChange = (itemId: string, isChecked: boolean) => { | |
| setSelectedItems((prev) => { | |
| const newSelectedItems = { ...prev, [itemId]: isChecked }; | |
| const newSelectedCount = Object.values(newSelectedItems).filter(Boolean).length; | |
| setOpen(newSelectedCount > 0); | |
| return newSelectedItems; | |
| }); | |
| }; |
| "prop": "render", | ||
| "type": [ | ||
| "React.ReactElement", | ||
| "(props: PopupProps, state: PopupState) => React.ReactElement" | ||
| ], | ||
| "default": "undefined", | ||
| "description": "팝업 요소를 렌더링하는 함수 또는 컴포넌트입니다." |
There was a problem hiding this comment.
positionerPrimitiveProps의 render prop에 대한 설명이 "팝업 요소를 렌더링하는 함수 또는 컴포넌트입니다."로 되어 있어 popupPrimitiveProps의 설명과 동일하여 혼동을 줄 수 있습니다. 이 prop은 positioner를 렌더링하므로, 설명을 "Positioner 요소를 렌더링하는 함수 또는 컴포넌트입니다."와 같이 수정하는 것이 명확하겠습니다.
| const options = [ | ||
| { id: 'item1', label: 'First Item', defaultChecked: false }, | ||
| { id: 'item2', label: 'Second Item', defaultChecked: false }, | ||
| { id: 'item3', label: 'Third Item', defaultChecked: false }, | ||
| ]; |
There was a problem hiding this comment.
스타일 가이드 4.6. 상수 (Constants)에 따라, options 배열과 같은 상수는 as const를 사용하여 불변성을 보장하고 타입 추론을 강화하는 것이 좋습니다.1
| const options = [ | |
| { id: 'item1', label: 'First Item', defaultChecked: false }, | |
| { id: 'item2', label: 'Second Item', defaultChecked: false }, | |
| { id: 'item3', label: 'Third Item', defaultChecked: false }, | |
| ]; | |
| const options = [ | |
| { id: 'item1', label: 'First Item', defaultChecked: false }, | |
| { id: 'item2', label: 'Second Item', defaultChecked: false }, | |
| { id: 'item3', label: 'Third Item', defaultChecked: false }, | |
| ] as const; |
Style Guide References
Footnotes
-
상수는
as const를 사용하여 불변성 및 타입 추론을 강화하는 것이 권장됩니다. ↩
|
|
||
| import * as styles from './action-bar.css'; | ||
|
|
||
| export const ActionBarRoot = (props: ActionBarRoot.Props) => <Popover.Root {...props} />; |
There was a problem hiding this comment.
스타일 가이드 6.14.1. Export 방식의 예시(490라인)에 따라, 디버깅 편의성을 위해 컴파운드 컴포넌트의 각 부분에 displayName을 설정하는 것이 좋습니다.1 ActionBarRoot 컴포넌트에도 displayName을 추가하고, 형식은 Parent.Child (예: ActionBar.Root)로 지정하는 것이 일관성에 좋습니다.
| export const ActionBarRoot = (props: ActionBarRoot.Props) => <Popover.Root {...props} />; | |
| export const ActionBarRoot = (props: ActionBarRoot.Props) => <Popover.Root {...props} />; | |
| ActionBarRoot.displayName = 'ActionBar.Root'; |
Style Guide References
Footnotes
-
컴파운드 컴포넌트의 각 부분에
displayName을 설정하는 것은 디버깅 시 React DevTools에서 컴포넌트를 쉽게 식별할 수 있도록 도와주므로 권장됩니다.Parent.Child형식을 사용하면 계층 구조를 명확히 할 수 있습니다. ↩
| return <Popover.Trigger ref={ref} {...componentProps} />; | ||
| }, | ||
| ); | ||
| ActionBarTrigger.displayName = 'ActionBarTrigger'; |
There was a problem hiding this comment.
스타일 가이드 6.14.1. Export 방식의 예시(490, 497라인)를 보면 displayName을 Parent.Child 형식(예: Dialog.Trigger)으로 지정하고 있습니다.1 이렇게 하면 React DevTools에서 컴포넌트 계층 구조를 더 쉽게 파악할 수 있습니다. 다른 하위 컴포넌트들의 displayName도 이 형식에 맞게 일관성 있게 수정하는 것을 권장합니다.
| ActionBarTrigger.displayName = 'ActionBarTrigger'; | |
| ActionBarTrigger.displayName = 'ActionBar.Trigger'; |
Style Guide References
Footnotes
-
컴파운드 컴포넌트의
displayName은Parent.Child형식으로 지정하여 React DevTools에서 컴포넌트의 소속과 역할을 명확히 나타내는 것이 권장됩니다. ↩
|
✅ All tests passed!
Click here if you need to update snapshots. |
noahchoii
changed the title
ActionBar componentFloatingBar component
| export namespace FloatingBarTrigger { | ||
| export type Props = VComponentProps<typeof Popover.Trigger>; | ||
| } | ||
|
|
||
| export namespace FloatingBarClose { | ||
| export type Props = VComponentProps<typeof Popover.Close>; | ||
| } | ||
|
|
||
| export namespace FloatingBarPortalPrimitive { | ||
| export type Props = VComponentProps<typeof Popover.Portal>; | ||
| } | ||
|
|
||
| export namespace FloatingBarPositionerPrimitive { | ||
| export type Props = VComponentProps<'div'>; | ||
| } | ||
|
|
||
| export namespace FloatingBarPopupPrimitive { | ||
| export type Props = VComponentProps<typeof Popover.Popup>; | ||
| } | ||
|
|
||
| export namespace FloatingBarPopup { | ||
| export type Props = FloatingBarPopupPrimitive.Props; | ||
| } |
There was a problem hiding this comment.
[Suggestion] I recommend replacing the namespace usage with standard named type exports.
- Standardization: Modern TypeScript development strongly prefers ES Modules over namespaces. As per the TypeScript Handbook and eslint-plugin-typescript, namespaces are considered legacy for code organization.
- Bundle Size: Namespaces compile to IIFEs or objects in JavaScript, which can hurt tree-shaking and add unnecessary runtime overhead compared to zero-cost type aliases or interfaces.
Proposed Change: Instead of FloatingBarRoot.Props, consider using a flat naming convention like FloatingBarRootProps.
There was a problem hiding this comment.
Thanks for the insightful review regarding standardization and bundle size optimization!
- Regarding Standardization & DX I fully acknowledge that
namespaceis often considered legacy in modern TypeScript development. However, this design choice was intentional to improve Cohesion and Developer Experience (DX).
If we rely solely on named exports (e.g., FloatingBarTriggerProps), consumers must rely on naming conventions to associate the props with FloatingBar.Trigger. By utilizing Declaration Merging, FloatingBar.Trigger acts as both the runtime component and the namespace for its types. This allows consumers to access types via a predictable, scoped path like FloatingBar.Trigger.Props, creating a explicit structural link rather than just a nominal one.
- Regarding Bundle Size You are correct that namespaces containing values compile to IIFEs. However, the namespaces used here are strictly Type-only namespaces. I have verified that these are completely erased during compilation and do not generate any runtime code (IIFEs) or objects. Therefore, there is zero runtime overhead and no negative impact on tree-shaking.
Given that this approach incurs no performance cost while significantly enhancing the semantic connection between the component and its API, would you be open to keeping this structure?
There was a problem hiding this comment.
I've verified with tsup that type-only namespaces indeed have no side effects, confirming your explanation.
While this isn't a common pattern in most design systems (e.g., Radix UI, Ark UI use flat exports), I can see the DX benefits of the Component.SubComponent.Props pattern for API cohesion.
If your team finds this pattern intuitive, it's worth keeping. However, I'd recommend:
- Adding a brief comment or documentation explaining this architectural choice
- This helps onboard new contributors who might expect the more common
ComponentPropspattern
| paddingInline: vars.size.space['200'], | ||
|
|
||
| borderRadius: vars.size.borderRadius[300], | ||
| border: `1px solid ${vars.color.border.normal}`, |
There was a problem hiding this comment.
It must be unified using 1px or 0.0625rem in border later.
Description of Changes
divelement, excluding custom props likesideandalign.initialFocus, it should be implemented when the version is upgraded.Checklist
Before submitting the PR, please make sure you have checked all of the following items.