DataList component #1047
DataList component #1047
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataList
participant DataListRoot
participant DataListItem
participant DataListLabel
participant DataListValue
participant DataListContext
User->>DataList: Use DataList.Root, DataList.Item, etc.
DataListRoot->>DataListContext: Provide rootClass via context
DataListRoot->>DataListItem: Render children
DataListItem->>DataListContext: Consume rootClass
DataListItem->>DataListLabel: Render label
DataListItem->>DataListValue: Render value
DataListLabel->>DataListContext: Consume rootClass
DataListValue->>DataListContext: Consume rootClass
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 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: 6
🧹 Nitpick comments (18)
src/components/ui/DataList/fragments/DataListValue.tsx (1)
5-8: Add better TypeScript typing for propsThe component's props typing can be improved to be more explicit about what props are accepted.
-const DataListValue = ({ children, className = '', ...props }: { children: React.ReactNode, className?: string }) => { +interface DataListValueProps extends React.HTMLAttributes<HTMLElement> { + children: React.ReactNode; + className?: string; +} + +const DataListValue = ({ children, className = '', ...props }: DataListValueProps) => { const { rootClass } = useContext(DataListContext); return <dd className={clsx(`${rootClass}-value`, className)} {...props}>{children}</dd>; };This makes it clear that the component accepts all standard HTML attributes for the
ddelement.src/components/ui/DataList/fragments/DataListItem.tsx (3)
2-2: Fix typo in import pathThere appears to be a typo in the import path - "DataListContex" should likely be "DataListContext".
-import DataListContext from '../contexts/DataListContex'; +import DataListContext from '../contexts/DataListContext';
5-8: Add error handling for context usageThe component doesn't handle the case where
DataListContextmight not be available. Consider adding a fallback value.const DataListItem = ({ children, className = '', ...props }: { children: React.ReactNode, className?: string }) => { - const { rootClass } = useContext(DataListContext); + const { rootClass = 'rad-ui-data-list' } = useContext(DataListContext); return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>; };
5-5: Improve type definitions with interface or type aliasConsider extracting the props type definition to improve readability and reusability.
+interface DataListItemProps { + children: React.ReactNode; + className?: string; + [key: string]: any; +} + -const DataListItem = ({ children, className = '', ...props }: { children: React.ReactNode, className?: string }) => { +const DataListItem = ({ children, className = '', ...props }: DataListItemProps) => {src/components/ui/DataList/fragments/DataListLabel.tsx (2)
2-2: Fix typo in import pathThere appears to be a typo in the import path - "DataListContex" should likely be "DataListContext".
-import DataListContext from '../contexts/DataListContex'; +import DataListContext from '../contexts/DataListContext';
5-5: Improve type definitions with interface or type aliasConsider extracting the props type definition to improve readability and reusability.
+interface DataListLabelProps { + children: React.ReactNode; + className?: string; + [key: string]: any; +} + -const DataListLabel = ({ children, className = '', ...props }: { children: React.ReactNode, className?: string }) => { +const DataListLabel = ({ children, className = '', ...props }: DataListLabelProps) => {styles/themes/components/data-list.scss (3)
6-6: Remove unnecessary blank lineThere is an extra blank line that can be removed.
gap: 16px; - .rad-ui-data-list-item {
8-8: Fix spacing in CSS propertyAdd a space after the colon in the display property for consistent formatting.
- display:grid; + display: grid;
13-19: Consider extracting repeated stylesThe label and value elements share identical font-size, line-height, and letter-spacing properties. Consider extracting these to a common selector to reduce duplication.
.rad-ui-data-list-label { grid-column: 1; color: var(--rad-ui-color-gray-900); - font-size: 14px; - line-height: 20px; - letter-spacing: 0.1px; } .rad-ui-data-list-value { grid-column: 2; color: var(--rad-ui-color-gray-1000); - font-size: 14px; - line-height: 20px; - letter-spacing: 0.1px; } + + .rad-ui-data-list-label, + .rad-ui-data-list-value { + font-size: 14px; + line-height: 20px; + letter-spacing: 0.1px; + }Also applies to: 21-27
src/components/ui/DataList/fragments/DataListRoot.tsx (4)
2-2: Fix typo in import pathThere appears to be a typo in the import path - "DataListContex" should likely be "DataListContext".
-import DataListContext from '../contexts/DataListContex'; +import DataListContext from '../contexts/DataListContext';
8-8: Improve type definitions with interface or type aliasConsider extracting the props type definition to improve readability and reusability.
+interface DataListRootProps { + children: React.ReactNode; + className?: string; + customRootClass?: string; + [key: string]: any; +} + -const DataListRoot = ({ children, className = '', customRootClass = '', ...props }: { children: React.ReactNode, className?: string, customRootClass?: string }) => { +const DataListRoot = ({ children, className = '', customRootClass = '', ...props }: DataListRootProps) => {
9-9: Document the purpose of customClassSwitcherAdd a comment explaining what customClassSwitcher does and how it determines the class name, as this might not be immediately obvious to new developers.
+ // Generate the root class name based on custom class or default using COMPONENT_NAME const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
10-15: Consider memoizing the context valueThe context value object is recreated on every render. Consider using useMemo to optimize performance, especially if the component might re-render frequently.
+ const contextValue = React.useMemo(() => ({ + rootClass + }), [rootClass]); + return <DataListContext.Provider - value={{ - rootClass - }}> + value={contextValue}> <div className={clsx(rootClass, className)} {...props}>{children}</div> </DataListContext.Provider>;src/components/ui/DataList/DataList.tsx (2)
1-4: Consolidate fragment imports for clarity
All four imports come from./fragments. Consider adding anindex.tsinfragments/that re-exports these, then import them in a single statement:// fragments/index.ts export { default as DataListRoot } from './DataListRoot'; export { default as DataListItem } from './DataListItem'; export { default as DataListLabel } from './DataListLabel'; export { default as DataListValue } from './DataListValue'; // DataList.tsx import { DataListRoot, DataListItem, DataListLabel, DataListValue, } from './fragments';This reduces boilerplate and makes future additions easier.
11-16: Group static assignments with export or useObject.assign
Mutating the exported function after theexport defaultcan be less clear. You could instead:-export default DataList; - -DataList.Root = DataListRoot; -DataList.Item = DataListItem; -DataList.Label = DataListLabel; -DataList.Value = DataListValue; +export default Object.assign(DataList, { + Root: DataListRoot, + Item: DataListItem, + Label: DataListLabel, + Value: DataListValue, +});This keeps the default export and its subcomponents declarations in one place.
src/components/ui/DataList/stories/DataList.stories.tsx (3)
7-10: Update story title and metadata
The title is currently prefixed withWIP/. Consider moving this out of the WIP namespace once the component is stable (e.g.,Components/DataList). You may also expand themetaexport withargTypesor asubtitleif needed.
14-16: Type the story function for better IntelliSense
Import and use theStorytype from@storybook/reactto annotate your story:-import React, { useState } from 'react'; +import React from 'react'; +import { Story, Meta } from '@storybook/react'; -export const Default = () => { +export const Default: Story = () => {This improves editor autocomplete for story arguments and controls.
42-46: Break up or scroll long content for readability
The placeholder paragraph is very long. Consider splitting it into multiple<p>tags, truncating it, or wrapping it in a scrollable container to keep the layout manageable in Storybook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/ui/DataList/DataList.tsx(1 hunks)src/components/ui/DataList/contexts/DataListContex.tsx(1 hunks)src/components/ui/DataList/fragments/DataListItem.tsx(1 hunks)src/components/ui/DataList/fragments/DataListLabel.tsx(1 hunks)src/components/ui/DataList/fragments/DataListRoot.tsx(1 hunks)src/components/ui/DataList/fragments/DataListValue.tsx(1 hunks)src/components/ui/DataList/stories/DataList.stories.tsx(1 hunks)styles/themes/components/badge.scss(1 hunks)styles/themes/components/data-list.scss(1 hunks)styles/themes/default.scss(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
styles/themes/components/badge.scss (1)
7-7: Good improvement to display propertyChanging from
flextoinline-fleximproves how the badge component will behave when used alongside other inline elements, particularly within the new DataList component. This allows badges to flow more naturally with surrounding content while maintaining the alignment benefits of flexbox.styles/themes/default.scss (1)
10-10: LGTM: New DataList component styles importThe import follows the project's alphabetical ordering convention and correctly adds the new DataList component styles to the default theme.
src/components/ui/DataList/fragments/DataListLabel.tsx (1)
5-8:✅ Verification successful
Consider semantic markup consistency
The component uses a
dtelement which is typically paired withddin description lists. Ensure this is intentional and that theDataListValuecomponent uses the correspondingddelement to maintain proper semantic HTML.
🏁 Script executed:
#!/bin/bash # Check if DataListValue uses the dd element to pair with dt grep -A 5 "return <dd" $(find src/components/ui/DataList -name "*.tsx")Length of output: 840
Semantic markup is consistent
DataListLabel correctly uses
<dt>, and DataListValue returns a matching<dd>. No changes are needed.src/components/ui/DataList/stories/DataList.stories.tsx (2)
18-24: Basic DataList usage looks correct
The markup correctly demonstratesDataList.Root,DataList.Item,DataList.Label, andDataList.Value. This effectively shows how to compose the component.
36-39: EmbeddingBadgeinsideDataList.Valueis valid
Using<Badge>here showcases integration with other UI elements—this will help consumers see how to embed richer content.
| import { createContext } from 'react'; | ||
|
|
||
| const DataListContext = createContext({}); | ||
|
|
||
| export default DataListContext; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in filename and add TypeScript interface
There's a typo in the filename: DataListContex.tsx should be DataListContext.tsx. Additionally, the context is created without type definitions which can lead to type errors when consuming this context.
Consider adding proper TypeScript typing:
-import { createContext } from 'react';
+import { createContext } from 'react';
-const DataListContext = createContext({});
+interface DataListContextType {
+ rootClass?: string;
+}
+
+const DataListContext = createContext<DataListContextType>({
+ rootClass: ''
+});
export default DataListContext;Also, please rename the file to correct the typo.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createContext } from 'react'; | |
| const DataListContext = createContext({}); | |
| export default DataListContext; | |
| import { createContext } from 'react'; | |
| interface DataListContextType { | |
| rootClass?: string; | |
| } | |
| const DataListContext = createContext<DataListContextType>({ | |
| rootClass: '' | |
| }); | |
| export default DataListContext; |
| import { clsx } from 'clsx'; | ||
|
|
||
| const DataListValue = ({ children, className = '', ...props }: { children: React.ReactNode, className?: string }) => { | ||
| const { rootClass } = useContext(DataListContext); |
There was a problem hiding this comment.
Handle potential undefined rootClass
The context is initialized with an empty object, but the component destructures rootClass without checking if it exists.
-const { rootClass } = useContext(DataListContext);
+const { rootClass = 'rad-ui-data-list' } = useContext(DataListContext);This provides a fallback value if the context doesn't provide rootClass. Alternatively, ensure the context is properly initialized with a default rootClass value as suggested in the context file review.
| @@ -0,0 +1,10 @@ | |||
| import React, { useContext } from 'react'; | |||
| import DataListContext from '../contexts/DataListContex'; | |||
There was a problem hiding this comment.
Fix import path typo
The import path contains a typo in the context filename.
-import DataListContext from '../contexts/DataListContex';
+import DataListContext from '../contexts/DataListContext';Be sure to fix the actual filename as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import DataListContext from '../contexts/DataListContex'; | |
| -import DataListContext from '../contexts/DataListContex'; | |
| +import DataListContext from '../contexts/DataListContext'; |
|
|
||
| .rad-ui-data-list-item { | ||
| display:grid; | ||
| grid-template-columns: subgrid; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider browser compatibility for subgrid
The subgrid value for grid-template-columns is not fully supported across all browsers. Consider adding a fallback or checking if this is a requirement for your supported browsers.
🌐 Web query:
What browsers support CSS subgrid?
💡 Result:
CSS Subgrid Browser Support Overview
CSS Subgrid is a feature of CSS Grid Layout that allows nested grids (child grid containers) to inherit their row and/or column definitions from their parent grid, providing powerful alignment capabilities for complex layouts.
Desktop Browser Support
| Browser | Supported Versions | Unsupported Versions |
|---|---|---|
| Chrome | 118 and above[1][3] | 4 – 117[1] |
| Edge | 118 and above[1][3] | 12 – 117[1] |
| Firefox | 71 and above[1][3] | 2 – 70[1] |
| Safari | 16.5 and above[1][7] | 3.2 – 15.2[1][7] |
| Opera | 103 and above[1] | 9.5 – 102[1] |
| Internet Explorer | Not supported[1] | All versions[1] |
Mobile Browser Support
| Browser | Supported Versions | Unsupported Versions |
|---|---|---|
| Safari on iOS | 16.2 and above[1][7] | 3.2 – 15[1] |
| Chrome for Android | 118 and above[1] | 97 – 117[1] |
| Firefox for Android | All versions[1] | — |
| Samsung Internet | 24 and above[1] | 4 – 23[1] |
| Opera Mobile | 80[1] | 10 – 73[1] |
| Android Browser | 118 and above[1] | 2.3 – 117[1] |
Summary & Additional Notes
- CSS Subgrid is now fully supported in all major modern browsers on both desktop and mobile, except for Internet Explorer, which does not support it at all[1][3][7].
- Earlier, subgrid support was only available in Firefox and later Safari, but recent updates have brought support to Chrome (from version 118), Edge (from version 118), and Opera (from version 103)[1][3][6].
- To ensure compatibility, especially for users on older browser versions, developers should use feature queries (like
@supports (grid-template-columns: subgrid)) to detect support and provide fallback styles as needed[2][5].
"CSS Subgrid shows a browser compatibility score of 75. This is a collective score out of 100 to represent browser support of a web technology. The higher this score is, the greater is the browser compatibility." [1]
In conclusion: As of April 2025, CSS Subgrid is fully supported in recent versions of Chrome, Edge, Firefox, Safari (including iOS), and Opera, making it broadly available for modern web development[1][3][7]. However, always check the minimum required version for your target audience and use progressive enhancement where appropriate.
Citations:
- 1: https://www.lambdatest.com/web-technologies/css-subgrid
- 2: https://web.dev/articles/css-subgrid
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout/Subgrid
- 4: https://www.browserstack.com/guide/what-are-css-subgrids
- 5: https://blog.pixelfreestudio.com/getting-tripped-up-by-css-subgrid-common-pitfalls-explained/
- 6: https://12daysofweb.dev/2022/css-subgrid/
- 7: https://www.lambdatest.com/web-technologies/css-subgrid-safari
Add feature-query–based fallback for CSS Subgrid
While modern browsers (Chrome 118+, Edge 118+, Firefox 71+, Safari 16.5+, Opera 103+, and their mobile equivalents) now support subgrid, older browsers—including Internet Explorer and versions prior to those listed—do not. To ensure a robust experience:
- Wrap your subgrid rule in a feature query:
@supports (grid-template-columns: subgrid) { .your-data-list { grid-template-columns: subgrid; } }
- Provide a default layout outside the
@supportsblock or in an@supports notblock. For example:.your-data-list { /* Fallback for older browsers */ grid-template-columns: repeat(3, 1fr); } @supports (grid-template-columns: subgrid) { .your-data-list { grid-template-columns: subgrid; } }
This ensures users on unsupported browsers see a reasonable grid, while modern browsers get the full benefits of subgrid.
| const DataList = () => { | ||
| console.warn('Direct usage of DataList is not supported. Please use DataList.Root, DataList.Item, etc. instead.'); | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit typing and guard console warning
Attaching static properties (Root, Item, etc.) to a plain React.FC will cause a TS error:
Property 'Root' does not exist on type '() => JSX.Element | null'
Define an interface and type DataList accordingly, and wrap console.warn so it only fires in development:
+// At top:
+import React from 'react';
+
+type DataListComponent = React.FC & {
+ Root: typeof DataListRoot;
+ Item: typeof DataListItem;
+ Label: typeof DataListLabel;
+ Value: typeof DataListValue;
+};
+
-const DataList = () => {
+const DataList: DataListComponent = () => {
- console.warn('Direct usage of DataList is not supported. Please use DataList.Root, DataList.Item, etc. instead.');
+ if (process.env.NODE_ENV === 'development') {
+ console.warn(
+ 'Direct usage of DataList is not supported. Please use DataList.Root, DataList.Item, etc.'
+ );
+ }
return null;
};This ensures TypeScript recognizes the static props and prevents spamming warnings in production.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DataList = () => { | |
| console.warn('Direct usage of DataList is not supported. Please use DataList.Root, DataList.Item, etc. instead.'); | |
| return null; | |
| }; | |
| import React from 'react'; | |
| type DataListComponent = React.FC & { | |
| Root: typeof DataListRoot; | |
| Item: typeof DataListItem; | |
| Label: typeof DataListLabel; | |
| Value: typeof DataListValue; | |
| }; | |
| const DataList: DataListComponent = () => { | |
| if (process.env.NODE_ENV === 'development') { | |
| console.warn( | |
| 'Direct usage of DataList is not supported. Please use DataList.Root, DataList.Item, etc.' | |
| ); | |
| } | |
| return null; | |
| }; |
| @@ -0,0 +1,51 @@ | |||
| import { Meta } from '@storybook/react'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
Remove unused useState import
useState is imported but never used in this story file. Please remove it to keep imports clean.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/DataList/contexts/DataListContex.tsx (1)
1-11: Fix typo in filename and add documentation commentsThere's a typo in the filename:
DataListContex.tsxshould beDataListContext.tsx. While you've implemented the TypeScript interface correctly, the filename still needs to be corrected.Also, consider adding JSDoc comments to explain the purpose of this context and how it should be used.
+/** + * Context for the DataList component + * Provides consistent class naming throughout the DataList component hierarchy + */ interface DataListContextType { rootClass?: string; } +/** + * DataList context with default rootClass value + * Used by DataList subcomponents to maintain consistent styling + */ const DataListContext = createContext<DataListContextType>({ rootClass: 'DataList' });
🧹 Nitpick comments (2)
src/components/ui/DataList/tests/DataList.test.tsx (2)
42-64: Consider simplifying custom class naming propsThe test shows that the Root component accepts both a standard
classNameprop and acustomRootClassprop, which seems redundant and could be confusing to users. Consider simplifying to use just one pattern for applying custom class names across all components.Also, since the class names are derived from the context's
rootClassvalue, it would be good to explicitly test that the value is being properly used as a prefix for item/label/value class names.test('should apply custom class names correctly', () => { const { container } = render( - <DataList.Root className="custom-root" customRootClass="custom-datalist"> + <DataList.Root className="custom-root" rootClass="custom-datalist"> <DataList.Item className="custom-item"> <DataList.Label className="custom-label">Phone</DataList.Label> <DataList.Value className="custom-value">123-456-7890</DataList.Value> </DataList.Item> </DataList.Root> );
1-102: Add tests for accessibility and edge casesThe current tests cover basic rendering and styling well, but consider adding:
- Accessibility tests to ensure proper ARIA attributes are applied
- Tests for edge cases like empty data or missing child components
- Tests validating that each component can only be used in the proper hierarchy
These additional tests would ensure the component is robust and follows accessibility best practices.
Example test for accessibility:
test('should have appropriate ARIA attributes', () => { const { container } = render( <DataList.Root aria-label="User Information"> <DataList.Item> <DataList.Label>Name</DataList.Label> <DataList.Value>John Doe</DataList.Value> </DataList.Item> </DataList.Root> ); const rootElement = container.firstChild; expect(rootElement).toHaveAttribute('aria-label', 'User Information'); // Add more expectations for other ARIA attributes as needed });Example test for edge cases:
test('should handle empty items gracefully', () => { render( <DataList.Root> <DataList.Item> <DataList.Label>Empty Value</DataList.Label> <DataList.Value></DataList.Value> </DataList.Item> </DataList.Root> ); expect(screen.getByText('Empty Value')).toBeInTheDocument(); // Verify the Value component renders properly even when empty });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/DataList/contexts/DataListContex.tsx(1 hunks)src/components/ui/DataList/stories/DataList.stories.tsx(1 hunks)src/components/ui/DataList/tests/DataList.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/DataList/stories/DataList.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/components/ui/DataList/tests/DataList.test.tsx (3)
19-40: LGTM! Good testing of class namesThe test covers the default class names appropriately, ensuring that each subcomponent receives its proper class.
66-90: LGTM! Thorough testing for multiple itemsGood job testing the multiple item scenario, which ensures the component can handle real-world use cases with multiple data points.
92-101: LGTM! Good test for direct usage warningGreat job adding a test to verify the warning behavior when using DataList directly. This helps enforce the correct usage pattern and catches developer errors early.
Summary by CodeRabbit