feat(SelectableCards): new component thumbnail variant #1445
Conversation
WalkthroughThe pull request introduces several new components and enhancements to the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 22
🧹 Outside diff range and nitpick comments (27)
packages/react-components/src/components/SelectableCard/index.ts (1)
2-9: Consider restructuring grid components and fix import path.
- Grid components (
GridWrapper,GridCol,GridRow) seem tightly coupled withInteractiveSelectableCard. Consider moving them to a separate module if they're meant to be reusable.- Remove the trailing slash from the import path.
export { ThumbnailSelectableCard, GallerySelectableCard, InteractiveSelectableCard, GridWrapper, GridCol, GridRow, -} from './components/'; +} from './components';packages/react-components/src/components/Avatar/Avatar.tsx (1)
Line range hint
148-148: Consider documenting style precedenceThe style merging logic (
{ ...backgroundStyle, ...style }) means custom styles will override background styles. Consider documenting this behavior in the props interface./** * Custom style for the avatar + * Note: Custom styles will override default background styles */ style?: React.CSSProperties;packages/react-components/src/hooks/types.ts (1)
20-28: Consider strengthening the MouseEvent type safetyThe MouseEvent type could be more specific by using a union type of allowed HTML elements instead of the generic HTMLElement.
- onClick: (e: React.MouseEvent<HTMLElement, MouseEvent>) => void; + onClick: (e: React.MouseEvent<HTMLDivElement | HTMLSpanElement, MouseEvent>) => void;packages/react-components/src/hooks/useInteractive.ts (1)
5-5: Expand NON_INTERACTIVE_TAGS listConsider adding other common interactive elements: 'label', 'summary', 'details'.
-const NON_INTERACTIVE_TAGS = ['input', 'button', 'select', 'textarea', 'a']; +const NON_INTERACTIVE_TAGS = ['input', 'button', 'select', 'textarea', 'a', 'label', 'summary', 'details'];packages/react-components/src/components/SelectableCard/SelectableCard.module.scss (1)
64-67: Use spacing variables for heightsReplace fixed height values with design system spacing variables.
- height: 21px; + height: var(--spacing-6); &__checkbox { - height: 16px; + height: var(--spacing-5); }packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.tsx (1)
15-15: Use classnames library for safer class compositionCurrent className concatenation could lead to extra spaces and potential undefined classes.
+import classNames from 'classnames'; // ... -<div className={`${styles[gridWrapperBaseClass]} ${className || ''}`}> +<div className={classNames(styles?.[gridWrapperBaseClass], className)}>packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/types.ts (1)
3-4: Document the purpose of empty interface extensionConsider adding JSDoc comments explaining why this interface exists if it doesn't add new properties.
packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.module.scss (1)
8-10: Consider using CSS variable for icon heightReplace the fixed height with a CSS variable for better maintainability and flexibility.
&__icon { display: flex; - height: 21px; + height: var(--thumbnail-icon-height, 21px); }packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/types.ts (1)
5-18: LGTM! Consider enhancing prop descriptions.The interface is well-structured and properly extends the core props. Consider adding examples in the JSDoc comments for better developer experience.
/** * The label of the card + * @example "Gallery Item 1" */ label?: string;packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.module.scss (1)
5-21: LGTM! Consider adding responsive behavior.The grid implementation is clean and follows best practices. Consider adding media queries for responsive gap adjustments on smaller screens.
.#{$grid-wrapper-base-class} { display: flex; flex-wrap: wrap; gap: var(--spacing-5) var(--spacing-16); + @media (max-width: 768px) { + gap: var(--spacing-4) var(--spacing-8); + } }packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/types.ts (1)
5-23: Consider creating shared types for common props.The interface is well-defined, but there's prop overlap with GallerySelectableCard. Consider extracting common props (label, icon, customElement) into a shared interface.
+interface ICommonCardProps { + label: string; + icon?: React.ReactNode; + customElement?: React.ReactNode; +} + -export interface IThumbnailSelectableCardProps extends ISelectableCardCoreProps { +export interface IThumbnailSelectableCardProps extends ISelectableCardCoreProps, ICommonCardProps { - label: string; description?: string; - icon?: React.ReactNode; - customElement?: React.ReactNode; }packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.tsx (1)
13-17: Add prop type validation for childrenConsider adding a more specific type for children to prevent invalid content.
export const InteractiveSelectableCard: FC<IInteractiveSelectableCardProps> = ({ children, contentClassName, ...props -}) => { +}: { + children: ReactNode; + contentClassName?: string; +}) => {packages/react-components/src/components/SelectableCard/types.ts (1)
28-28: Add default kind typeConsider specifying a default kind in the type to match component's default behavior.
- kind?: 'thumbnail' | 'gallery' | 'interactive'; + kind?: 'thumbnail' | 'gallery' | 'interactive' | 'default';packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.stories.tsx (2)
3-7: Consider using absolute importsReplace relative imports (
../../../../stories/components/StoryDescriptor) with absolute imports for better maintainability.
18-38: Enhance story coverageConsider adding examples for:
- Disabled state
- Error state
- Selected state
- Different sizes
packages/react-components/src/components/SelectableCard/SelectableCard.stories.css (1)
23-24: Avoid fixed dimensionsConsider using CSS custom properties or relative units for width/height to improve component flexibility.
packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.tsx (1)
14-21: Add prop validation for required propsConsider adding prop-types or TypeScript required markers for essential props like
label.packages/react-components/src/components/SelectableCard/SelectableCard.stories.tsx (1)
30-58: Enhance story maintainability
- Extract selection logic into a custom hook
- Move inline styles to CSS module
- Use array mapping for cards instead of repetition
export const Default = () => { const [selectedIndex, setSelectedIndex] = React.useState<number>(1); + const cards = ['Default selectable card 1', 'Default selectable card 2', 'Default selectable card 3']; return ( - <div style={{ display: 'flex', gap: 8 }}> + <div className={styles.cardContainer}> + {cards.map((text, index) => ( + <SelectableCard + key={index} + selectionType="radio" + isSelected={selectedIndex === index + 1} + onClick={() => setSelectedIndex(index + 1)} + > + {text} + </SelectableCard> + ))} </div> ); };packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.stories.tsx (1)
18-50: Reduce duplication and enhance story coverage
- Create a shared story template for both radio and checkbox examples
- Add interaction tests using play functions
const Template = (selectionType: 'radio' | 'checkbox') => ( <> <StoryDescriptor title="With Label"> <Cards selectionType={selectionType} /> </StoryDescriptor> {/* ... other variants ... */} </> ); export const RadioTypeExamples = () => Template('radio'); export const CheckboxTypeExamples = () => Template('checkbox');packages/react-components/src/components/SelectableCard/SelectableCard.tsx (1)
39-48: Simplify keyboard event handlingMultiple spacebar key checks can be simplified.
- if ( - e.key === 'Enter' || - e.key === ' ' || - e.key === 'Spacebar' || - e.key === 'Space' - ) { + if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onClick?.(); }packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.spec.tsx (1)
19-69: Add test coverage for edge casesConsider adding tests for:
- Error handling scenarios
- Accessibility attributes
- Keyboard interactions
Example test cases to add:
it('should handle keyboard interactions', () => { const onClick = vi.fn(); const { getByRole } = renderComponent({ ...DEFAULT_PROPS, onClick, }); const button = getByRole('button'); fireEvent.keyDown(button, { key: 'Enter' }); expect(onClick).toHaveBeenCalled(); }); it('should have proper aria attributes', () => { const { getByRole } = renderComponent({ ...DEFAULT_PROPS, label: 'Test Label', }); const button = getByRole('button'); expect(button).toHaveAttribute('aria-selected', 'false'); expect(button).toHaveAttribute('aria-label', 'Test Label'); });packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.components.tsx (2)
21-45: Refactor getComponent for better maintainabilityExtract custom element to a separate component and avoid inline styles. Consider using styled-components or CSS modules.
+ const CustomElement = ({ icon: Icon, index }: { icon: IconComponent, index: number }) => ( + <StyledCustomElement> + <Icon size="small" /> + <div>{`Custom element ${index + 1}`}</div> + </StyledCustomElement> + );
76-91: Optimize selection logic using SetCurrent array operations can be inefficient for large datasets.
- const [selectedIndex, setSelectedIndex] = React.useState<number[]>([]); + const [selectedIndices, setSelectedIndices] = React.useState<Set<number>>(new Set()); const handleCardClick = (index: number) => { - if (selectedIndex.includes(index)) { - setSelectedIndex(selectedIndex.filter((selected) => selected !== index)); + setSelectedIndices(prev => { + const next = new Set(prev); + if (next.has(index)) { + next.delete(index); + } else { + next.add(index); + } + return next; + }); - } else { - setSelectedIndex([...selectedIndex, index]); - } };packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx (1)
1-104: Consider component composition patternCreate a base SelectableCardGroup component that both ThumbnailSelectableCard and GallerySelectableCard can extend.
This would:
- Reduce code duplication
- Improve maintainability
- Make it easier to add new card variants
packages/react-components/src/components/SelectableCard/SelectableCard.spec.tsx (1)
88-98: Enhance keyboard interaction test coverageConsider adding tests for space key press and tab+shift navigation for complete keyboard accessibility coverage.
it('should allow to focus by tab press and call onClick on keyboard interaction', () => { const onClick = vi.fn(); renderComponent({ ...DEFAULT_PROPS, onClick, }); userEvent.tab(); userEvent.keyboard('{enter}'); + userEvent.keyboard(' '); expect(onClick).toHaveBeenCalledTimes(1); + + // Test backward navigation + userEvent.tab({ shift: true }); + expect(document.activeElement).not.toBe(getByRole('button')); });packages/react-components/src/components/SelectableCard/SelectableCard.mdx (1)
127-127: Fix grammar in InteractiveGrid descriptionCorrect subject-verb agreement.
-The grid components consists of: +The grid components consist of:🧰 Tools
🪛 LanguageTool
[grammar] ~127-~127: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: -GridWrapper- necessary to use ...(SUBJECT_VERB_AGREEMENT_PLURAL)
packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.stories.tsx (1)
15-15: Consider moving styles to a component-specific fileThe story-specific styles should be colocated with the stories rather than using the parent's CSS file.
-import '../../SelectableCard.stories.css'; +import './InteractiveSelectableCard.stories.css';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (36)
packages/react-components/src/components/Avatar/Avatar.tsx(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.mdx(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.spec.tsx(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.stories.css(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.components.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.spec.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/types.ts(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/index.ts(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.components.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.spec.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/types.ts(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.spec.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/types.ts(1 hunks)packages/react-components/src/components/SelectableCard/components/index.ts(1 hunks)packages/react-components/src/components/SelectableCard/index.ts(1 hunks)packages/react-components/src/components/SelectableCard/types.ts(1 hunks)packages/react-components/src/hooks/index.ts(1 hunks)packages/react-components/src/hooks/types.ts(2 hunks)packages/react-components/src/hooks/useInteractive.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)packages/react-components/src/utils/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/index.ts
- packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.module.scss
- packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.module.scss
🧰 Additional context used
🪛 LanguageTool
packages/react-components/src/components/SelectableCard/SelectableCard.mdx
[grammar] ~127-~127: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: - GridWrapper - necessary to use ...
(SUBJECT_VERB_AGREEMENT_PLURAL)
🔇 Additional comments (11)
packages/react-components/src/components/SelectableCard/index.ts (1)
1-1: LGTM!
Clean and straightforward export of the base component.
packages/react-components/src/utils/types.ts (1)
7-17: LGTM! Props ordering follows React conventions
The addition of children prop and repositioning of style prop follows React's conventional prop ordering, with children coming first.
packages/react-components/src/components/Avatar/Avatar.tsx (1)
56-59: LGTM! Style prop addition is well documented
The style prop addition follows TypeScript best practices and includes clear documentation.
packages/react-components/src/index.ts (1)
45-45: LGTM! Export follows established patterns
The new SelectableCard export is correctly placed in alphabetical order.
packages/react-components/src/hooks/index.ts (1)
4-4: LGTM!
The export follows the established pattern and maintains consistency with other hook exports.
packages/react-components/src/components/SelectableCard/SelectableCard.module.scss (3)
26-32: LGTM! Consistent spacing and positioning
The thumbnail variant follows design system conventions with proper spacing variables.
55-59: LGTM! Well-handled selected state
Good use of design tokens and proper handling of both normal and hover states.
13-13: Consider container width constraints
Using max-content might cause overflow issues in narrow containers. Consider using a more flexible width strategy.
packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.tsx (1)
1-37: Document props and verify test coverage
Consider adding:
- JSDoc comments for each component
- Prop type documentation
- Unit tests for className composition and children rendering
packages/react-components/src/components/SelectableCard/components/index.ts (1)
1-8: LGTM! Clean and well-organized exports
The exports are logically grouped and follow a consistent pattern.
packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.tsx (1)
20-23: Review accessibility role usage
The role="presentation" might strip semantic meaning from interactive elements. Consider using role="group" if the div contains interactive elements.
✅ Verification successful
role="presentation" is appropriate here
The role="presentation" is correctly used in this case because:
- The div is just a styling wrapper
- The parent
SelectableCardcomponent already handles all interactive behavior with properrole="button"and keyboard interactions - No semantic meaning is being stripped as the wrapper div contains no interactive elements of its own
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for interactive elements within this component
ast-grep --pattern 'role="presentation"' -A 5
Length of output: 47
Script:
#!/bin/bash
# Let's check the full component implementation to understand the context
ast-grep --pattern 'InteractiveSelectableCard = $$$'
# Also check for any button, input, or other interactive elements
rg -A 10 "InteractiveSelectableCard" packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.tsx
Length of output: 688
Script:
#!/bin/bash
# Let's check what's inside the children prop and the full component content
rg -l "InteractiveSelectableCard" --type tsx
cat packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.tsx
Length of output: 846
Script:
#!/bin/bash
# Let's check the parent SelectableCard component and its implementation
cat packages/react-components/src/components/SelectableCard/SelectableCard.tsx
# Also check where InteractiveSelectableCard is being used
rg -B 2 -A 2 "InteractiveSelectableCard" --type ts --type tsx
Length of output: 1927
packages/react-components/src/components/SelectableCard/SelectableCard.module.scss
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/SelectableCard/SelectableCard.module.scss
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/SelectableCard/SelectableCard.module.scss
Outdated
Show resolved
Hide resolved
| describe('<ThumbnailSelectableCard> component', () => { | ||
| it('should allow for custom class for card wrapper and content', () => { | ||
| const { container, getByRole } = renderComponent({ | ||
| ...DEFAULT_PROPS, | ||
| className: 'my-class', | ||
| contentClassName: 'my-content-class', | ||
| }); | ||
|
|
||
| expect(container.firstChild).toHaveClass('my-class'); | ||
| expect(getByRole('presentation')).toHaveClass('my-content-class'); | ||
| }); | ||
|
|
||
| it('should render with label', () => { | ||
| const { getByText } = renderComponent(DEFAULT_PROPS); | ||
|
|
||
| expect(getByText('Label')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render with description if provided', () => { | ||
| const { getByText } = renderComponent({ | ||
| ...DEFAULT_PROPS, | ||
| description: 'Description', | ||
| }); | ||
|
|
||
| expect(getByText('Description')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render with icon if provided', () => { | ||
| const { getByRole } = renderComponent({ | ||
| ...DEFAULT_PROPS, | ||
| icon: <div role="img" />, | ||
| }); | ||
|
|
||
| expect(getByRole('img')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should not render label and other elements if custom element is provided', () => { | ||
| const { getByText, queryByRole, queryByText } = renderComponent({ | ||
| ...DEFAULT_PROPS, | ||
| description: 'Description', | ||
| icon: <div role="img" />, | ||
| customElement: <div>Custom element</div>, | ||
| }); | ||
|
|
||
| expect(queryByRole('img')).not.toBeInTheDocument(); | ||
| expect(queryByText('Label')).not.toBeInTheDocument(); | ||
| expect(queryByText('Description')).not.toBeInTheDocument(); | ||
| expect(getByText('Custom element')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for critical functionality
While basic rendering is well covered, the test suite should include:
- Selection behavior (radio/checkbox modes)
- Click handler verification
- Edge cases with null/undefined props
- Accessibility checks (ARIA attributes, keyboard navigation)
...ponents/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.components.tsx
Show resolved
Hide resolved
...nts/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/src/components/SelectableCard/SelectableCard.mdx
Outdated
Show resolved
Hide resolved
...ts/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/react-components/src/components/SelectableCard/SelectableCard.mdx (2)
71-71: Fix typo in prop name
customElemntshould becustomElement🧰 Tools
🪛 LanguageTool
[typographical] ~71-~71: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... provide aniconorcustomElemntorlabel, otherwise the card will be empty. <Canvas of={Ga...(THUS_SENTENCE)
135-135: Fix grammar in grid components descriptionChange "The grid components consists of" to "The grid components consist of"
🧰 Tools
🪛 LanguageTool
[grammar] ~135-~135: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: -GridWrapper- necessary to use ...(SUBJECT_VERB_AGREEMENT_PLURAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/react-components/src/components/SelectableCard/SelectableCard.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/react-components/src/components/SelectableCard/SelectableCard.mdx
[typographical] ~71-~71: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... provide an icon or customElemnt or label, otherwise the card will be empty. <Canvas of={Ga...
(THUS_SENTENCE)
[grammar] ~135-~135: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: - GridWrapper - necessary to use ...
(SUBJECT_VERB_AGREEMENT_PLURAL)
🔇 Additional comments (2)
packages/react-components/src/components/SelectableCard/SelectableCard.mdx (2)
60-61: Fix arrow function syntax in onClick handlers
The arrow function syntax is incorrect in multiple examples.
- onClick={() = {}}
+ onClick={() => {}}Also applies to: 86-87, 107-108
1-144: Well-structured documentation with comprehensive examples
The documentation provides clear explanations and examples for all SelectableCard variants. The inclusion of implementation examples and API documentation makes it very useful for developers.
🧰 Tools
🪛 LanguageTool
[typographical] ~71-~71: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... provide an icon or customElemnt or label, otherwise the card will be empty. <Canvas of={Ga...
(THUS_SENTENCE)
[grammar] ~135-~135: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: - GridWrapper - necessary to use ...
(SUBJECT_VERB_AGREEMENT_PLURAL)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/types.ts (3)
6-10: Enhance BaseProps documentationThe label property documentation could be more descriptive. Consider adding details about its purpose and usage context.
/** - * The label of the card + * The text label displayed in the gallery card. This label helps identify + * the card's content for users. */
17-20: Fix incorrect documentation for never propThe documentation for
customElementis misleading since this prop can never be used in the WithIcon type.- /** - * The custom element of the card - */ + /** @deprecated This prop cannot be used with WithIcon type */
24-27: Fix incorrect documentation for never propThe documentation for
iconis misleading since this prop can never be used in the WithCustomElement type.- /** - * The icon of the card - */ + /** @deprecated This prop cannot be used with WithCustomElement type */packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx (1)
59-69: Make card count configurableThe number of cards is currently hardcoded to
4. Consider adding a prop to make the card count configurable for greater flexibility.Also applies to: 93-103
packages/react-components/src/components/SelectableCard/SelectableCard.mdx (1)
135-135: Fix grammatical errorChange "The grid components consists of:" to "The grid components consist of:" to match the plural subject.
Apply this diff to correct the grammar:
- The grid components consists of: + The grid components consist of:🧰 Tools
🪛 LanguageTool
[grammar] ~135-~135: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: -GridWrapper- necessary to use ...(SUBJECT_VERB_AGREEMENT_PLURAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
packages/react-components/src/components/SelectableCard/SelectableCard.mdx(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.stories.css(1 hunks)packages/react-components/src/components/SelectableCard/SelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.components.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.module.scss(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.spec.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/types.ts(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.stories.tsx(1 hunks)packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx(1 hunks)packages/react-components/src/hooks/useInteractive.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react-components/src/hooks/useInteractive.ts
- packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.spec.tsx
- packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.tsx
- packages/react-components/src/components/SelectableCard/SelectableCard.tsx
- packages/react-components/src/components/SelectableCard/SelectableCard.stories.css
- packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.module.scss
- packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.stories.tsx
- packages/react-components/src/components/SelectableCard/SelectableCard.module.scss
- packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveGrid/InteractiveGrid.tsx
🧰 Additional context used
🪛 LanguageTool
packages/react-components/src/components/SelectableCard/SelectableCard.mdx
[grammar] ~135-~135: The verb form ‘consists’ does not seem to match the subject ‘components’.
Context: ...he UI assumptions. The grid components consists of: - GridWrapper - necessary to use ...
(SUBJECT_VERB_AGREEMENT_PLURAL)
🔇 Additional comments (4)
packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/types.ts (1)
34-35: LGTM! Well-structured type definitions
Excellent use of TypeScript's discriminated union pattern to ensure mutually exclusive props. The type structure effectively prevents invalid prop combinations while maintaining type safety.
packages/react-components/src/components/SelectableCard/components/ThumbnailSelectableCard/ThumbnailSelectableCard.components.tsx (1)
52-70: Refactor shared logic into a custom hook
The RadioCards and CheckboxCards components share similar state management and rendering logic. Extracting this into a custom hook or component will improve code reuse and maintainability.
Also applies to: 78-104
packages/react-components/src/components/SelectableCard/components/GallerySelectableCard/GallerySelectableCard.components.tsx (1)
67-76: Make card count configurable
The card count is hardcoded to 4. Introducing a prop to configure the number of cards can enhance the component's reusability.
Also applies to: 98-107
packages/react-components/src/components/SelectableCard/components/InteractiveSelectableCard/InteractiveSelectableCard.stories.tsx (1)
24-84: Reduce code duplication in story examples
The RadioTypeExamples and CheckboxTypeExamples stories have repetitive code. Extracting shared content into reusable components will improve maintainability.
Also applies to: 86-148
Resolves: #1347
Description
New
SelectableCardcomponent with dedicated components based on it:ThumbnailSelectableCardGallerySelectableCardInteractiveSelectableCardwith small grid system (GridWrapper,GridRow,GridCol)Docs page, unit tests, stories representing each variant.
Storybook
https://feature-1347--613a8e945a5665003a05113b.chromatic.com/?path=/docs/components-selectablecard--docs
Checklist
Obligatory:
Summary by CodeRabbit
Release Notes
New Features
styleprop for the Avatar component, allowing custom styling.SelectableCardcomponent with options for radio and checkbox selections.GallerySelectableCardandThumbnailSelectableCardcomponents for enhanced card selection interfaces.InteractiveSelectableCardfor interactive card functionality with customizable content.GridWrapper,GridRow, andGridColcomponents for responsive layouts.useInteractivehook for enhanced click handling.Documentation
SelectableCardand its variations in Storybook.Tests