-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(DataTableSkeleton): add size prop for consistent row sizing #21111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21111 +/- ##
==========================================
+ Coverage 92.62% 96.87% +4.24%
==========================================
Files 515 158 -357
Lines 38225 23711 -14514
Branches 5861 951 -4910
==========================================
- Hits 35406 22970 -12436
+ Misses 2670 731 -1939
+ Partials 149 10 -139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
adamalston
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the API snapshot?
| it('should respect the size prop', () => { | ||
| render(<DataTableSkeleton size="xs" />); | ||
|
|
||
| expect(screen.getByRole('table')).toHaveClass( | ||
| `${prefix}--data-table--xs` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest also including a check for the prop's default value.
| showToolbar?: boolean; | ||
|
|
||
|
|
||
| size?: DataTableSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this code been formatted?
| size?: DataTableSize; | |
| /* | |
| * Changes the row height of table. | |
| */ | |
| size?: DataTableSize; |
| showToolbar: PropTypes.bool, | ||
|
|
||
|
|
||
| size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl']), | |
| /* | |
| * Changes the row height of table. | |
| */ | |
| size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl']), |
|
@naaa760 Snapshot tests are failing due to mismatches. Run |
Closes: #21096
sizeprop toDataTableSkeletonto matchDataTablerow sizing (xs, sm, md, lg, xl) for visual consistency between loading and data states.Changelog
New
sizeprop toDataTableSkeletonwith test coverage