feat(Toggle): add new Toggle, ToggleGroup component#628
Conversation
🦋 Changeset detectedLatest commit: 7c6ee67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
|
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces two new components, ChangesToggle and ToggleGroup Components
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
|
✅ All tests passed!
Click here if you need to update snapshots. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/components/toggle/toggle.tsx (1)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
??over||for fallback.
sizeProp || contextSize/variantProp || contextVariantshould use nullish coalescing since these are enum-string props;??more precisely expresses "use context only when the prop is unset" and avoids accidental fallback if a future variant value were falsy.♻️ Proposed fix
- const size = sizeProp || contextSize; - const variant = variantProp || contextVariant; + const size = sizeProp ?? contextSize; + const variant = variantProp ?? contextVariant;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/toggle/toggle.tsx` around lines 24 - 25, Use nullish coalescing in toggle.tsx for the fallback logic in the Toggle component so only unset props fall back to context. Update the size and variant resolution in the Toggle implementation to replace the current `sizeProp`/`contextSize` and `variantProp`/`contextVariant` fallback behavior with `??`, keeping the logic in the same place and preserving the existing prop/context precedence in the Toggle component.Source: Coding guidelines
packages/core/src/components/toggle-group/toggle-group.tsx (1)
27-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winContext value doesn't adopt the recipe's resolved defaults; relies on two recipes'
defaultVariantsstaying in sync.
variantsPropsis derived purely fromcreateSplitProps, so when a consumer doesn't setsize/variantexplicitly,ToggleGroupProviderprovides{ size: undefined, variant: undefined }to descendants instead of the resolved'md'/'default'. This currently "works" only becausetoggle.css.ts's recipe happens to declare the samedefaultVariantsastoggle-group.css.ts's recipe, soToggle's ownstyles.root({size: undefined, ...})falls back to matching values. If either recipe's defaults diverge in the future, group and item styling will silently disagree.Consider resolving
variantsPropsagainst the recipe's own defaults (e.g. merge with{ size: 'md', variant: 'default' }or readroot.defaultVariants/use a shared constant) before passing it to the context provider, so the context value reflects the actually-applied style rather than raw possibly-undefinedprops.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/toggle-group/toggle-group.tsx` around lines 27 - 46, The ToggleGroup context is passing raw variant props with undefined values, so descendants don’t receive the recipe’s resolved defaults and can drift from the applied styling. Update ToggleGroupRoot to resolve variantsProps against the ToggleGroup recipe defaults before giving it to ToggleGroupProvider, using the same default source as toggle-group.css.ts (or a shared constant) so size and variant always reflect the actual applied values. Keep the existing createSplitProps flow, but merge in the defaultVariants before rendering BaseToggleGroup and providing context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/toggle-group/toggle-group.css.ts`:
- Around line 6-32: ToggleGroup의 `root`와 `separator` 스타일이 `data-orientation`를
반영하지 않아 `orientation="vertical"`에서 레이아웃이 깨집니다. `toggle-group.css.ts`의
`root`/`separator` 정의에 `data-orientation="vertical"` 분기를 추가해 `root`는 세로 배치로
전환하고, `separator`는 축과 여백을 세로 모드에 맞게 바꾸세요. `componentRecipe`, `componentStyle`,
`root`, `separator`를 기준으로 수정하면 됩니다.
---
Nitpick comments:
In `@packages/core/src/components/toggle-group/toggle-group.tsx`:
- Around line 27-46: The ToggleGroup context is passing raw variant props with
undefined values, so descendants don’t receive the recipe’s resolved defaults
and can drift from the applied styling. Update ToggleGroupRoot to resolve
variantsProps against the ToggleGroup recipe defaults before giving it to
ToggleGroupProvider, using the same default source as toggle-group.css.ts (or a
shared constant) so size and variant always reflect the actual applied values.
Keep the existing createSplitProps flow, but merge in the defaultVariants before
rendering BaseToggleGroup and providing context.
In `@packages/core/src/components/toggle/toggle.tsx`:
- Around line 24-25: Use nullish coalescing in toggle.tsx for the fallback logic
in the Toggle component so only unset props fall back to context. Update the
size and variant resolution in the Toggle implementation to replace the current
`sizeProp`/`contextSize` and `variantProp`/`contextVariant` fallback behavior
with `??`, keeping the logic in the same place and preserving the existing
prop/context precedence in the Toggle component.
🪄 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: edb8c3d3-215f-44ef-825d-eb3fd79bd519
⛔ Files ignored due to path filters (8)
packages/core/__tests__/screenshots/toggle--test-bed-1-chrome-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/toggle--test-bed-1-edge-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/toggle--test-bed-1-firefox-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/toggle--test-bed-1-safari-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/togglegroup--test-bed-1-chrome-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/togglegroup--test-bed-1-edge-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/togglegroup--test-bed-1-firefox-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/togglegroup--test-bed-1-safari-darwin-.pngis excluded by!**/*.png
📒 Files selected for processing (14)
.changeset/famous-bugs-guess.md.changeset/tricky-files-cheat.mdpackages/core/src/components/toggle-group/index.parts.tspackages/core/src/components/toggle-group/index.tspackages/core/src/components/toggle-group/toggle-group.css.tspackages/core/src/components/toggle-group/toggle-group.stories.tsxpackages/core/src/components/toggle-group/toggle-group.test.tsxpackages/core/src/components/toggle-group/toggle-group.tsxpackages/core/src/components/toggle/index.tspackages/core/src/components/toggle/toggle.css.tspackages/core/src/components/toggle/toggle.stories.tsxpackages/core/src/components/toggle/toggle.test.tsxpackages/core/src/components/toggle/toggle.tsxpackages/core/src/index.ts
| export const root = componentRecipe({ | ||
| base: { | ||
| display: 'inline-flex', | ||
| alignItems: 'center', | ||
| gap: vars.size.space['050'], | ||
|
|
||
| padding: vars.size.space['050'], | ||
|
|
||
| borderRadius: vars.size.borderRadius['400'], | ||
| border: '1px solid', | ||
| borderColor: vars.color.border.normal, | ||
| backgroundColor: vars.color.background.canvas[100], | ||
| }, | ||
|
|
||
| defaultVariants: { size: 'md', variant: 'default' }, | ||
| variants: { | ||
| size: { sm: {}, md: {}, lg: {}, xl: {} }, | ||
| variant: { default: {}, accent: {} }, | ||
| }, | ||
| }); | ||
|
|
||
| export const separator = componentStyle({ | ||
| width: '0.0625rem', | ||
| alignSelf: 'stretch', | ||
| marginBlock: vars.size.space['050'], | ||
| backgroundColor: vars.color.border.normal, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/core/src/components/toggle-group/*' && \
echo '--- outline: toggle-group.tsx ---' && \
ast-grep outline packages/core/src/components/toggle-group/toggle-group.tsx --view expanded && \
echo '--- outline: toggle-group.css.ts ---' && \
ast-grep outline packages/core/src/components/toggle-group/toggle-group.css.ts --view expanded && \
echo '--- search orientation usage ---' && \
rg -n --no-heading 'orientation|data-orientation|ToggleGroup' packages/core/src/components/toggle-groupRepository: goorm-dev/vapor-ui
Length of output: 8932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- toggle-group.tsx ---'
cat -n packages/core/src/components/toggle-group/toggle-group.tsx
echo '--- toggle-group.css.ts ---'
cat -n packages/core/src/components/toggle-group/toggle-group.css.ts
echo '--- toggle-group.test.tsx ---'
cat -n packages/core/src/components/toggle-group/toggle-group.test.tsx
echo '--- toggle-group.stories.tsx ---'
cat -n packages/core/src/components/toggle-group/toggle-group.stories.tsxRepository: goorm-dev/vapor-ui
Length of output: 20641
🌐 Web query:
@base-ui/react toggle-group orientation data-orientation horizontal vertical documentation
💡 Result:
In the @base-ui/react library, the ToggleGroup component accepts an orientation prop that determines the component's orientation, which defaults to 'horizontal' [1][2]. The orientation prop accepts the values 'horizontal' or 'vertical' [1]. When this prop is set, the ToggleGroup component updates its rendered data-orientation attribute on the underlying DOM element (e.g., data-orientation="horizontal" or data-orientation="vertical") [3]. It is important to note that, as of the current implementation, setting the orientation prop primarily affects the data-orientation attribute and accessibility metadata [3]. It does not currently constrain keyboard navigation behavior; the component's composite root typically handles navigation across both axes regardless of the specified orientation [3]. To use the orientation prop: Option 1 Option 2
Citations:
- 1: https://mui-base-ui.mintlify.app/components/toggle-group
- 2: https://github.com/mui/base-ui/blob/master/packages/react/src/toggle-group/ToggleGroup.tsx
- 3: ToggleGroup orientation prop does not affect keyboard navigation mui/base-ui#4487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/core/src/components/toggle-group/toggle-group.tsx')
text = p.read_text()
print('has aria-orientation strip:', 'aria-orientation={undefined}' in text)
print('base props extend full base ui props:', 'VaporUIComponentProps<typeof BaseToggleGroup, State>' in text)
PYRepository: goorm-dev/vapor-ui
Length of output: 232
orientation="vertical" 지원이 빠져 있습니다. ToggleGroup.Root가 Base UI의 orientation prop을 그대로 받지만, root/separator 스타일은 data-orientation를 전혀 반영하지 않아 세로 모드에서도 가로 배치 + 세로 구분선으로 렌더링됩니다. data-orientation="vertical" 분기에서 flexDirection과 separator 축을 바꿔 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/toggle-group/toggle-group.css.ts` around lines 6
- 32, ToggleGroup의 `root`와 `separator` 스타일이 `data-orientation`를 반영하지 않아
`orientation="vertical"`에서 레이아웃이 깨집니다. `toggle-group.css.ts`의 `root`/`separator`
정의에 `data-orientation="vertical"` 분기를 추가해 `root`는 세로 배치로 전환하고, `separator`는 축과
여백을 세로 모드에 맞게 바꾸세요. `componentRecipe`, `componentStyle`, `root`, `separator`를
기준으로 수정하면 됩니다.
| describe('keyboard interaction', () => { | ||
| it('should toggle pressed state when Space key is pressed', async () => { | ||
| const rendered = render(<ToggleTest />); | ||
| const toggle = rendered.getByRole('button', { name: LABEL_TEXT }); | ||
|
|
||
| await userEvent.tab(); | ||
| await userEvent.keyboard('[Space]'); | ||
|
|
||
| expect(toggle).toHaveAttribute('aria-pressed', 'true'); | ||
|
|
||
| await userEvent.keyboard('[Space]'); | ||
|
|
||
| expect(toggle).toHaveAttribute('aria-pressed', 'false'); | ||
| }); |
There was a problem hiding this comment.
혹시 키보드 인터렉션 space만 추가하신 이유가 있을까요? enter로도 가능해야 할 것 같은데 테스트케이스에는 없어서 추가 제안드립니다.!
There was a problem hiding this comment.
엇 사실 Space를 입력했을 때와 다르게 Enter를 입력했을 때는 hover 같은 상호작용 이벤트가 발생하지 않아서 이벤트가 발생하지 않는다고 생각했네요.. 케이스는 추가해두겠습니다!
| defaultVariants: { size: 'md', variant: 'default' }, | ||
| variants: { | ||
| size: { sm: {}, md: {}, lg: {}, xl: {} }, | ||
| variant: { default: {}, accent: {} }, |
There was a problem hiding this comment.
여기 variant가 들어간 이유가 있을까요? 시안에는 variant관련 속성이 없어서 여쭤봅니다.!
There was a problem hiding this comment.
아 이건 다른 Group 컴포넌트들처럼 그룹 전체에 variant를 적용하는 케이스를 추가해둔건데, 피그마에 반영 요청을 드리는 것을 잊었네요..! 디자이너분께 따로 요청 드리겠습니다!
| variant: { | ||
| control: { type: 'inline-radio' }, | ||
| options: ['default', 'accent'], | ||
| }, |
There was a problem hiding this comment.
이 속성 추가시 모든 toggle에 동일한 값이 들어가는 것 같은데, 이렇게 모든 toggle에 대해 일괄 적용해야 하는 케이스가 있었을까요??
| const children = useRenderElement({ | ||
| render: childrenRender, | ||
| props: { | ||
| 'aria-hidden': 'true', | ||
| className: styles.icon, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
이건 단순 질문입니다.!
이렇게 하면 aria-hidden은 svg 요소 뿐만 아니라, 다른 span 요소도 들어오게 됩니다.! 그럼 text의 경우 aria-hidden이 적용될 것으로 예상됩니다.
시안에도 아이콘만 적용되어 있던데, 지금 구조상 권장하는 방식은 icon만 children으로 들어 오는 것을 권장하는 것인가요??
그리고 여기 예시처럼 텍스트가 들어오는 경우도 있는데, 이렇게 쓰는 경우 사용자에게 aria-hidden 적용 가능성을 어떻게 전달하면 좋을까요?
There was a problem hiding this comment.
넵 우선은 IconButton과 마찬가지로 아이콘만 포함되는 것으로 계획했습니다! 아직까지 Toggle이 텍스트와 함께 사용되는 케이스가 없는 것으로 확인했고, 그런 상황에서 Text를 포함하는 케이스까지 고려하는 게 당장은 불필요하다고 결론을 내렸습니다!
그리고 Text와 Icon이 모두 포함되는 케이스 처리 방법에 대해 잠깐 고민해봤는데, 그런 케이스가 필요하다면 Toggle은 이제 어떤 요소든 포함될 수 있는 것으로 두고, Toggle.Icon처럼 아이콘만을 처리하는 별도 컴포넌트가 있으면 좋을 것 같아요!
3 participants
Description of Changes
Summary by CodeRabbit
New Features
Togglecontrol with size, variant, disabled, and pressed states.ToggleGroupcontrol with separator support and multi-select behavior.Tests
ToggleandToggleGroup.Documentation
Toggle,ToggleGroup컴포넌트를 추가했습니다.Checklist
Before submitting the PR, please make sure you have checked all of the following items.