feat(menu-sheet): replace dialog with drawer headless, remove close button#1301
Draft
junghyeonsu wants to merge 14 commits intodevfrom
Draft
feat(menu-sheet): replace dialog with drawer headless, remove close button#1301junghyeonsu wants to merge 14 commits intodevfrom
junghyeonsu wants to merge 14 commits intodevfrom
Conversation
…utton - Replace @seed-design/react-dialog with @seed-design/react-drawer as the underlying headless primitive for drag-to-close and mobile interaction support - Remove MenuSheetCloseButton component and its exported types - Add direction="bottom" default to MenuSheetRoot via Drawer.Root - Remove withStateProps usage (Drawer propagates state via data attributes) - Add touchAction: "none" and willChange: "transform" to content slot for Drawer compatibility - Remove closeButton slot from menu-sheet recipe Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ac0b387 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMenuSheet가 Dialog 기반에서 Drawer 기반 API로 전면 전환되었고, CloseButton 관련 API는 사용 중단 표기 또는 제거되었으며 레시피·문서·레지스트리·예제·드로어 훅의 일부 로직(shouldDrag)과 MenuSheetContent의 handle/footer 동작이 함께 변경되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Trigger as MenuSheet.Trigger
participant DrawerRoot as Drawer.Root
participant Positioner as Drawer.Positioner
participant Backdrop as Drawer.Backdrop
participant Content as Drawer.Content
participant Handle as MenuSheet.Handle
User->>Trigger: click/open
Trigger->>DrawerRoot: request open
DrawerRoot->>Positioner: position & mount content
Positioner->>Backdrop: mount backdrop
Positioner->>Content: mount content
Content->>Handle: render (if showHandle)
User->>Backdrop: click (optional)
Backdrop->>DrawerRoot: requestClose(reason)
User->>Content: drag (optional)
Content->>DrawerRoot: requestClose(reason=drag)
DrawerRoot->>Content: animateClose()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/src/components/MenuSheet/MenuSheet.tsx (1)
60-149:forwardRef컴포넌트에displayName을 추가해주세요.
MenuSheetContent,MenuSheetGroup,MenuSheetItem는forwardRef를 사용하지만displayName이 없어 DevTools 가독성과 디버깅 품질이 떨어집니다.As per coding guidelines, `**/*.{tsx,jsx}`: Use `forwardRef` and `displayName` for React components; and `packages/react/src/components/**/*.tsx`: All React components must include a `displayName` property after the component definition.제안 패치
export const MenuSheetContent = React.forwardRef<HTMLDivElement, MenuSheetContentProps>( @@ ); +MenuSheetContent.displayName = "MenuSheetContent"; @@ export const MenuSheetGroup = React.forwardRef<HTMLDivElement, MenuSheetGroupProps>( @@ ); +MenuSheetGroup.displayName = "MenuSheetGroup"; @@ export const MenuSheetItem = React.forwardRef<HTMLButtonElement, MenuSheetItemProps>( @@ ); +MenuSheetItem.displayName = "MenuSheetItem";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/MenuSheet/MenuSheet.tsx` around lines 60 - 149, Three forwardRef components lack displayName which hurts DevTools and debugging; add a displayName static property for MenuSheetContent, MenuSheetGroup, and MenuSheetItem immediately after each forwardRef declaration (e.g., MenuSheetContent.displayName = "MenuSheetContent", MenuSheetGroup.displayName = "MenuSheetGroup", MenuSheetItem.displayName = "MenuSheetItem") so they appear correctly in React DevTools and satisfy the repo's linting rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/MenuSheet/MenuSheet.tsx`:
- Around line 22-27: The Drawer-based MenuSheetRoot changed mount behavior
(content stays in DOM when closed); restore the previous Dialog semantics by
enabling unmountOnExit and lazyMount for the Drawer root (e.g., add
unmountOnExit: true and lazyMount: true into the withRootProvider defaultProps
for MenuSheetRoot or wrap Drawer.Root with the Presence component to preserve
mount/unmount behavior) and add displayName strings for the forwarded components
by assigning MenuSheetContent.displayName = "MenuSheetContent",
MenuSheetGroup.displayName = "MenuSheetGroup", and MenuSheetItem.displayName =
"MenuSheetItem" immediately after each forwardRef definition.
---
Nitpick comments:
In `@packages/react/src/components/MenuSheet/MenuSheet.tsx`:
- Around line 60-149: Three forwardRef components lack displayName which hurts
DevTools and debugging; add a displayName static property for MenuSheetContent,
MenuSheetGroup, and MenuSheetItem immediately after each forwardRef declaration
(e.g., MenuSheetContent.displayName = "MenuSheetContent",
MenuSheetGroup.displayName = "MenuSheetGroup", MenuSheetItem.displayName =
"MenuSheetItem") so they appear correctly in React DevTools and satisfy the
repo's linting rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d410296-bd0b-4c62-b5df-a6ab11cffef9
⛔ Files ignored due to path filters (9)
packages/css/all.cssis excluded by!packages/css/**/*packages/css/all.layered.cssis excluded by!packages/css/**/*packages/css/all.layered.min.cssis excluded by!packages/css/**/*packages/css/all.min.cssis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.cssis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.d.tsis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.layered.cssis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.layered.mjsis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.mjsis excluded by!packages/css/**/*
📒 Files selected for processing (4)
packages/qvism-preset/src/recipes/menu-sheet.tspackages/react/src/components/MenuSheet/MenuSheet.namespace.tspackages/react/src/components/MenuSheet/MenuSheet.tsxpackages/react/src/components/MenuSheet/index.ts
💤 Files with no reviewable changes (2)
- packages/react/src/components/MenuSheet/index.ts
- packages/react/src/components/MenuSheet/MenuSheet.namespace.ts
- Deprecate MenuSheetCloseButton (to be removed in 1.3.0): restore closeButton slot in recipe and re-export component/types with @deprecated JSDoc, wrapping Drawer.CloseButton for backward compatibility - Remove CloseButton from docs/registry/ui/menu-sheet.tsx example - Add MenuSheet CloseButton entry to deprecations.mdx Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deploying seed-design-stackflow-spa with
|
| Latest commit: |
40f6980
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68e16d9e.seed-design-stackflow-spa.pages.dev |
| Branch Preview URL: | https://claude-busy-noether.seed-design-stackflow-spa.pages.dev |
Contributor
Alpha Preview (Stackflow SPA)
|
Deploying seed-design-storybook with
|
| Latest commit: |
40f6980
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4cca7dee.seed-design-storybook.pages.dev |
| Branch Preview URL: | https://claude-busy-noether.seed-design-storybook.pages.dev |
Contributor
Alpha Preview (Storybook)
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/content/docs/migration/deprecations.mdx (1)
13-13: 대체안 컬럼에 마이그레이션 안내 추가 고려현재 대체안이 "-"로 표기되어 있으나, 코드의 JSDoc에는
Drawer.CloseButton을 직접 사용하라는 안내가 있습니다. 사용자가 문서만 참고할 경우를 대비해 대체안 또는 비고에 이 정보를 추가하면 좋겠습니다.제안된 수정
-| MenuSheet - CloseButton | 컴포넌트 | 1.2.x | 1.3.0 | - | Drawer 기반으로 전환, 닫기 버튼 패턴 제거 | +| MenuSheet - CloseButton | 컴포넌트 | 1.2.x | 1.3.0 | Drawer.CloseButton | Drawer 기반으로 전환, 닫기 버튼 패턴 제거 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/migration/deprecations.mdx` at line 13, The table row for "MenuSheet - CloseButton" currently shows "-" in the replacement column; update that cell to include the migration guidance from the JSDoc by replacing "-" with a clear replacement note such as "Use Drawer.CloseButton" (optionally add a short remark like "Drawer 기반으로 전환 — use Drawer.CloseButton"); reference the symbol names MenuSheet - CloseButton and Drawer.CloseButton so readers can find the replacement easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/content/docs/migration/deprecations.mdx`:
- Line 13: The table row for "MenuSheet - CloseButton" currently shows "-" in
the replacement column; update that cell to include the migration guidance from
the JSDoc by replacing "-" with a clear replacement note such as "Use
Drawer.CloseButton" (optionally add a short remark like "Drawer 기반으로 전환 — use
Drawer.CloseButton"); reference the symbol names MenuSheet - CloseButton and
Drawer.CloseButton so readers can find the replacement easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1982903f-ea28-4b3f-8e92-756b600b06c7
⛔ Files ignored due to path filters (7)
docs/public/__registry__/ui/menu-sheet.jsonis excluded by!**/__registry__/**/*,!**/public/**/*packages/css/all.cssis excluded by!packages/css/**/*packages/css/all.layered.cssis excluded by!packages/css/**/*packages/css/all.layered.min.cssis excluded by!packages/css/**/*packages/css/all.min.cssis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.cssis excluded by!packages/css/**/*packages/css/recipes/menu-sheet.layered.cssis excluded by!packages/css/**/*
📒 Files selected for processing (6)
docs/content/docs/migration/deprecations.mdxdocs/registry/ui/menu-sheet.tsxpackages/qvism-preset/src/recipes/menu-sheet.tspackages/react/src/components/MenuSheet/MenuSheet.namespace.tspackages/react/src/components/MenuSheet/MenuSheet.tsxpackages/react/src/components/MenuSheet/index.ts
💤 Files with no reviewable changes (1)
- docs/registry/ui/menu-sheet.tsx
Deploying seed-design-v3 with
|
| Latest commit: |
40f6980
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3de080e5.seed-design.pages.dev |
| Branch Preview URL: | https://claude-busy-noether.seed-design.pages.dev |
Contributor
Alpha Preview (Docs)
|
The shouldDrag function previously checked for role="dialog" only inside the scrollHeight > clientHeight block. When menu-sheet dialog content was short (not scrollable), the check was never reached, causing the DOM walk to continue up to the <html> element. If the page was scrolled down, html.scrollTop > 0 would block drag-to-close. Move the role="dialog" check outside the scrollable block so it acts as a boundary regardless of whether the dialog content is scrollable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pdate docs - Create MenuSheetHandle component (reuses bottom-sheet-handle recipe) - Add showHandle (default: false) and showCloseButton (default: true) to registry - Update MenuSheetTitle with useDrawerContext for isCloseButtonRendered awareness - Fix drag-to-close: reorder role="dialog" check before scrollable check in shouldDrag - Update menu-sheet docs to mirror bottom-sheet structure (Trigger, Controlled, Show Handle, Show Close Button, Dismissible sections) - Add 5 new examples: trigger, controlled, show-handle, show-close-button, dismissible - Update stackflow-spa menu-sheet registry with same pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match BottomSheet pattern where Handle is only accessible via namespace (MenuSheet.Handle), not as a flat named export. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Handle is now shown by default (showHandle default: true) - Deprecated close button is hidden by default (showCloseButton default: false) - Update examples and docs to reflect new defaults Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BREAKING CHANGE: MenuSheetCloseButton component and closeButton recipe slot have been removed. Users who relied on the close button should implement their own dismiss button using Drawer.CloseButton. - Remove MenuSheetCloseButton component, props, and exports - Remove closeButton slot from menu-sheet recipe - Remove showCloseButton prop from registry and stackflow examples - Remove show-close-button example - Remove closeButton reason from onOpenChange docs - Move CloseButton to "removed" in deprecations.mdx - Default showHandle to true (handle shown by default) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tangy-banks-knock.md:
- Around line 2-4: This changeset incorrectly marks breaking API removals as
minor; update the entries so that `@seed-design/react` and `@seed-design/css` are
bumped to major instead of minor because exports
MenuSheet.CloseButton/MenuSheetCloseButton and the menu-sheet slot closeButton
were removed; edit the .changeset/tangy-banks-knock.md diff to change the
version level for "@seed-design/react" and "@seed-design/css" from minor to
major and keep the note about the BREAKING CHANGE describing the removed
symbols.
In `@docs/examples/react/menu-sheet/controlled.tsx`:
- Line 26: 현재 예제에서 MenuSheetContent에 title="Actions"와 aria-label이 동시에 지정되어 접근성
이름이 중복되므로 aria-label을 제거해 주세요: MenuSheetContent 컴포넌트 호출부(문맥: MenuSheetContent
title="Actions" aria-label="Menu Sheet")에서 aria-label prop을 삭제하고 title만 사용하도록
수정하여 accessible name이 중복되지 않게 만드세요.
In `@docs/examples/react/menu-sheet/dismissible.tsx`:
- Line 20: The example uses MenuSheetContent with title="Actions" but also
passes aria-label="Menu Sheet", causing visible title and accessible name to
differ; remove the aria-label prop from the MenuSheetContent instance (the
attribute aria-label="Menu Sheet") in dismissible.tsx so the accessible name
comes from the title prop and matches the visible heading.
In `@examples/stackflow-spa/src/seed-design/ui/menu-sheet.tsx`:
- Around line 58-64: The fallback hidden title currently renders whenever title
is falsy, which can produce an empty SeedMenuSheet.Title when consumers supply
only aria-labelledby; change the conditional so the VisuallyHidden +
SeedMenuSheet.Title fallback is rendered only if title is falsy AND
otherProps["aria-label"] is truthy; update the conditional around
SeedMenuSheet.Title/VisuallyHidden to check both title and
otherProps["aria-label"] so we don't emit an empty title that breaks accessible
name computation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b756e1f2-bb58-4e9d-89a2-73ce22d833a1
⛔ Files ignored due to path filters (3)
docs/public/__registry__/ui/menu-sheet.jsonis excluded by!**/__registry__/**/*,!**/public/**/*packages/css/all.cssis excluded by!packages/css/**/*packages/css/all.layered.cssis excluded by!packages/css/**/*
📒 Files selected for processing (9)
.changeset/tangy-banks-knock.mddocs/content/docs/migration/deprecations.mdxdocs/content/react/components/menu-sheet.mdxdocs/examples/react/menu-sheet/controlled.tsxdocs/examples/react/menu-sheet/dismissible.tsxdocs/examples/react/menu-sheet/show-handle.tsxdocs/examples/react/menu-sheet/trigger.tsxdocs/registry/ui/menu-sheet.tsxexamples/stackflow-spa/src/seed-design/ui/menu-sheet.tsx
✅ Files skipped from review due to trivial changes (2)
- docs/examples/react/menu-sheet/show-handle.tsx
- docs/examples/react/menu-sheet/trigger.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/registry/ui/menu-sheet.tsx
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@seed-design/react-dialogwith@seed-design/react-draweras the underlying headless primitive, enabling drag-to-close and mobile interaction supportMenuSheetCloseButtoncomponent and all related exported typescloseButtonslot from themenu-sheetrecipetouchAction: "none"andwillChange: "transform"tocontentslot for Drawer compatibilityMotivation
BottomSheet already uses
@seed-design/react-drawerfor better mobile UX (drag-to-close, touch gestures). MenuSheet shares the same sheet pattern, so it should use the same headless primitive for consistency.Test plan
Summary by CodeRabbit
Breaking Changes
New Features / Changes
Documentation