Add Segment Group #1017
Add Segment Group #1017
Conversation
🦋 Changeset detectedLatest commit: bd30342 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 Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/docs/src/components/demos/segment-group/disabled.tsxOops! Something went wrong! :( ESLint: 9.19.0 Error: Cannot find package '/node_modules/@tszhong0411/eslint-config/dist/index.js' imported from /eslint.config.mjs apps/docs/src/components/demos/segment-group/segment-group.tsxOops! Something went wrong! :( ESLint: 9.19.0 Error: Cannot find package '/node_modules/@tszhong0411/eslint-config/dist/index.js' imported from /eslint.config.mjs apps/docs/src/components/demos/segment-group/vertical.tsxOops! Something went wrong! :( ESLint: 9.19.0 Error: Cannot find package '/node_modules/@tszhong0411/eslint-config/dist/index.js' imported from /eslint.config.mjs
WalkthroughThis pull request introduces a new Segment Group component to the Changes
Sequence DiagramsequenceDiagram
participant User
participant SegmentGroup
participant SegmentGroupItem
User->>SegmentGroup: Initialize with orientation
SegmentGroup->>SegmentGroupItem: Create items
SegmentGroupItem-->>SegmentGroup: Render items
User->>SegmentGroup: Select item
SegmentGroup->>SegmentGroupItem: Update selected state
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
apps/docs/src/components/demos/segment-group/segment-group.tsx (2)
12-18: Add onChange handler for better interactivityConsider adding an
onChangehandler to track segment changes and demonstrate the component's interactive capabilities.- <SegmentGroup defaultValue='react'> + <SegmentGroup + defaultValue='react' + onChange={(details) => console.log('Selected:', details.value)} + >
3-8: Consider adding aria-labels for better accessibilityThe framework options could benefit from more descriptive aria-labels.
const frameworks = [ - { label: 'React', value: 'react' }, - { label: 'Solid', value: 'solid' }, - { label: 'Svelte', value: 'svelte' }, - { label: 'Vue', value: 'vue' } + { label: 'React', value: 'react', ariaLabel: 'React Framework' }, + { label: 'Solid', value: 'solid', ariaLabel: 'Solid Framework' }, + { label: 'Svelte', value: 'svelte', ariaLabel: 'Svelte Framework' }, + { label: 'Vue', value: 'vue', ariaLabel: 'Vue Framework' } ]apps/docs/src/components/demos/segment-group/vertical.tsx (1)
3-8: Consider extracting frameworks data to a shared constantThe frameworks array is duplicated across demo files. Consider moving it to a shared constants file to maintain DRY principles.
Create a new file
apps/docs/src/components/demos/segment-group/constants.ts:export const FRAMEWORKS = [ { label: 'React', value: 'react' }, { label: 'Solid', value: 'solid' }, { label: 'Svelte', value: 'svelte' }, { label: 'Vue', value: 'vue' } ] as constapps/docs/src/components/demos/segment-group/disabled.tsx (2)
3-8: Consider making disabled state more dynamicThe disabled state is currently hardcoded. Consider making it more dynamic to better demonstrate various use cases.
const frameworks = [ { label: 'React', value: 'react' }, { label: 'Solid', value: 'solid' }, - { label: 'Svelte', value: 'svelte', disabled: true }, + { label: 'Svelte', value: 'svelte', disabled: process.env.NODE_ENV === 'production' }, { label: 'Vue', value: 'vue' } ]
12-22: Add disabled state feedbackConsider adding visual feedback or tooltip when users attempt to interact with disabled items.
<SegmentGroup defaultValue='react'> {frameworks.map((framework) => ( <SegmentGroupItem key={framework.value} value={framework.value} disabled={framework.disabled} + title={framework.disabled ? 'This option is currently unavailable' : undefined} > {framework.label} </SegmentGroupItem> ))} </SegmentGroup>packages/ui/src/segment-group.tsx (2)
6-23: LGTM! Consider enhancing accessibility.The SegmentGroup implementation is well-structured with proper props handling and conditional styling.
Consider adding ARIA attributes for better accessibility:
<SegmentGroupPrimitive.Root orientation={orientation} + role="tablist" + aria-orientation={orientation} className={cn( 'flex items-start', orientation === 'horizontal' ? 'gap-4 border-b' : 'flex-col gap-1 border-l', className )} {...rest} >
45-65: LGTM! Consider adding keyboard navigation styles.The implementation handles states well and includes proper accessibility elements.
Consider adding keyboard focus styles for better navigation:
className={cn( 'text-muted-foreground hover:text-accent-foreground cursor-pointer font-medium transition-colors', 'data-[orientation=horizontal]:px-1 data-[orientation=horizontal]:pb-3', 'data-[orientation=vertical]:px-3 data-[orientation=vertical]:py-1.5', 'data-[state=checked]:text-foreground', 'data-[disabled]:text-muted-foreground data-[disabled]:cursor-not-allowed data-[disabled]:opacity-50', + 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2', className )}.changeset/swift-phones-relax.md (1)
1-5: Consider adding more details to the changeset.While the patch version is appropriate, the description could be more informative.
Consider expanding the description:
--- '@tszhong0411/ui': patch --- -Add Segment Group component +Add Segment Group component + +- Introduces new SegmentGroup component with horizontal/vertical orientations +- Includes support for disabled states and keyboard navigation +- Provides proper accessibility features via ARIA attributes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/swift-phones-relax.md(1 hunks)apps/docs/src/app/ui/components/segment-group.mdx(1 hunks)apps/docs/src/components/demos/segment-group/disabled.tsx(1 hunks)apps/docs/src/components/demos/segment-group/segment-group.tsx(1 hunks)apps/docs/src/components/demos/segment-group/vertical.tsx(1 hunks)apps/docs/src/config/links.ts(1 hunks)package.json(2 hunks)packages/ui/src/index.ts(1 hunks)packages/ui/src/segment-group.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/src/app/ui/components/segment-group.mdx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: TypeScript
- GitHub Check: Knip
- GitHub Check: Tests
- GitHub Check: ESLint
🔇 Additional comments (5)
packages/ui/src/index.ts (1)
42-42: LGTM!The segment-group export is correctly added and maintains the alphabetical ordering of exports.
packages/ui/src/segment-group.tsx (1)
27-41: LGTM! Clean implementation of the indicator.The indicator component effectively uses data attributes and CSS variables for dynamic positioning and dimensions.
apps/docs/src/config/links.ts (1)
169-172: LGTM! Properly integrated into navigation.The Segment Group component link is correctly added and maintains alphabetical ordering.
package.json (2)
36-36: LGTM! Improved prepare script.Adding package builds to the prepare script ensures dependencies are properly built during installation.
82-82: Verify the tsup downgrade impact.The downgrade from tsup 8.3.6 to 8.3.0 might affect build outputs.
✅ Verification successful
The tsup downgrade from 8.3.6 to 8.3.0 is safe.
All changes between these versions are non-breaking bug fixes and improvements, following semantic versioning principles. The core build functionality remains unchanged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes between tsup versions npm view [email protected] version npm view [email protected] version # Check for any reported issues gh api graphql -f query=' { repository(owner: "egoist", name: "tsup") { issues(states: OPEN, first: 5, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { title url createdAt } } } }'Length of output: 1148
Script:
#!/bin/bash # Check release notes and commits between versions gh api graphql -f query=' { repository(owner: "egoist", name: "tsup") { releases(first: 10, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { tagName description createdAt } } } }' # Get commit history between versions gh api repos/egoist/tsup/compare/v8.3.0...v8.3.6Length of output: 66635
Description
Added Segment Group.
Type of Change
Pre-flight Checklist
pnpm checkwithout any issuesScreenshots
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements