feat(suite): Add orientation for SelectBar component #16844
Conversation
WalkthroughThe pull request implements support for orientation customization in the SelectBar component. It introduces a new Tip 🌐 Web search-backed reviews and chat
🪧 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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
packages/components/src/components/form/SelectBar/SelectBar.tsx (2)
256-282: Add keyboard navigation support for vertical orientation.The keyboard navigation should handle both horizontal and vertical orientations:
const handleKeyboardNav = (e: KeyboardEvent) => { const selectedOptionIndex = options.findIndex(option => option.value === selectedOptionIn); let option; - if (e.key === 'ArrowLeft') { + if (e.key === 'ArrowLeft' || (orientation === 'vertical' && e.key === 'ArrowUp')) { const previousIndex = selectedOptionIndex - 1; if (previousIndex >= 0) { option = options[previousIndex]; } else { option = options[options.length - 1]; } - } else if (e.key === 'ArrowRight') { + } else if (e.key === 'ArrowRight' || (orientation === 'vertical' && e.key === 'ArrowDown')) { const previousIndex = selectedOptionIndex + 1; if (previousIndex <= options.length - 1) { option = options[previousIndex]; } else { [option] = options; } }
296-309: Add ARIA attributes for better accessibility.The component should include proper ARIA attributes to improve accessibility for screen readers:
<Options $optionsCount={options.length} $isFullWidth={isFullWidth} $elevation={elevation} $orientation={orientation} + role="radiogroup" + aria-orientation={orientation === 'vertical' ? 'vertical' : 'horizontal'} > <Puck $optionsCount={options.length} $selectedIndex={selectedIndex} $elevation={nextElevation[elevation]} $orientation={orientation} tabIndex={0} onKeyDown={handleKeyboardNav} + aria-hidden="true" />Also add ARIA attributes to the Option component:
<Option key={String(option.value)} onClick={handleOptionClick(option)} $isDisabled={!!isDisabled} $isSelected={selectedOptionIn !== undefined ? selectedOptionIn === option.value : false} data-testid={`select-bar/${String(option.value)}`} + role="radio" + aria-checked={selectedOptionIn === option.value} + aria-disabled={isDisabled} >
🧹 Nitpick comments (3)
packages/components/src/components/form/SelectBar/SelectBar.tsx (3)
29-32: Add JSDoc comments for better documentation.Consider adding JSDoc comments to explain the purpose and usage of these exported constants and types.
+/** Valid orientation values for the SelectBar component */ export const orientations = ['horizontal', 'vertical', 'auto'] as const; +/** Type representing the possible orientation values */ export type Orientation = (typeof orientations)[number]; +/** Default orientation value for the SelectBar component */ export const DEFAULT_ORIENTATION = 'auto';
47-71: Simplify columnPuck implementation and add comments.The CSS helpers could be improved for better maintainability:
+// Styles for vertical orientation wrapper const columnWrapper = css` flex-direction: column; align-items: flex-start; width: 100%; `; +// Styles for vertical orientation options container const columnOption = css` grid-auto-flow: row; width: 100%; border-radius: ${borders.radii.lg}; `; -const columnPuck = ({ - $selectedIndex: selectedIndex, - $optionsCount, -}: { - $optionsCount: number; - $selectedIndex: number; -}) => css` +// Styles for vertical orientation puck +const columnPuck = css<{ $optionsCount: number; $selectedIndex: number }>` left: 4px; right: 4px; top: 4px; width: auto; - height: ${getPuckWidth($optionsCount)}; - transform: ${`translateY(${getTranslateValue(selectedIndex)})`}; + height: ${({ $optionsCount }) => getPuckWidth($optionsCount)}; + transform: ${({ $selectedIndex }) => `translateY(${getTranslateValue($selectedIndex)})`}; `;
72-157: Extract media queries and simplify vertical styles.The styled components could be improved for better maintainability and reusability:
+const belowSmBreakpoint = css` + ${breakpointMediaQueries.below_sm} { + ${columnWrapper} + } +`; const Wrapper = styled.div< TransientProps<AllowedFrameProps> & { $isFullWidth?: boolean; $orientation: Orientation } >` /* ... existing styles ... */ ${({ $orientation }) => - $orientation === 'auto' && - css` - ${breakpointMediaQueries.below_sm} { - ${columnWrapper} - } - `} + $orientation === 'auto' && belowSmBreakpoint} ${({ $orientation }) => $orientation === 'vertical' && columnWrapper} `; /* Apply similar changes to Options and Puck components */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/src/components/form/SelectBar/SelectBar.stories.tsx(3 hunks)packages/components/src/components/form/SelectBar/SelectBar.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (1)
packages/components/src/components/form/SelectBar/SelectBar.stories.tsx (1)
4-9: LGTM! Well-structured Storybook configuration.The orientation control is properly implemented with appropriate defaults and radio button selection.
Also applies to: 26-26, 59-64
| ${({ $orientation }) => | ||
| $orientation === 'auto' && | ||
| css` | ||
| ${breakpointMediaQueries.below_sm} { | ||
| ${columnWrapper} | ||
| } | ||
| `} | ||
|
|
||
| ${({ $orientation }) => $orientation === 'vertical' && columnWrapper} |
There was a problem hiding this comment.
Not 100% happy with this, if you have better idea how to do it, feel free to comment it
Description
Related Issue
Resolve
Screenshots: