Exposes <TableCell> so users have more control over table markups #74
Conversation
`Warning: Invalid argument supplied to oneOfType. Expected an array of check functions, but received undefined at index 1.`
✅ Deploy Preview for vip-design-system-components ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
brunobasto
changed the title
brunobasto
changed the title
src/system/Table/TableCell.js
Outdated
| const TableCell = ( { head, isRowHead, children, ...rest } ) => { | ||
| let scope = null; | ||
|
|
||
| if ( head ) { |
There was a problem hiding this comment.
@brunobasto unrelated to this PR but I find the "head/isRowHead" a little bit confusing. Even by looking at the code I always wonder what they are used for.
I'm sure renaming them might not be the case now but I was wondering if we could add some extra example in the stories to clarify the use of the parameters
There was a problem hiding this comment.
I agree it's confusing. I'll play with the storybook and add some examples.
There was a problem hiding this comment.
So, essentially, head and isRowHead control two things:
- Whether to render the columns as
<td>or<th>. If either is true, the column will render as<th> - The scope of the column header:
The scope attribute specifies whether a header cell is a header for a column, row, or group of columns or rows.
Note: The scope attribute has no visual effect in ordinary web browsers, but can be used by screen readers.
I searched in vip-ui and couldn't find any usages of rowHead or isRowHead, so, for the sake of simplicity in the API, I removed isRowHead and, since we pass forward the props, if someone ever needs to control the scope for accessibility purposes, they can do:
<TableCell head scope="row">...There was a problem hiding this comment.
Since the prop I removed was unused (so it shouldn't impact the Dashboard) and the PR is already approved, I'll be bold and move forward with merging this, but we can add it back quickly if we need to.
| @@ -56,7 +56,7 @@ const Time = ( { time, relativeTime = false, timeOnly = false, className = null, | |||
| }; | |||
|
|
|||
| Time.propTypes = { | |||
| time: PropTypes.oneOfType( [ PropTypes.string, PropTypes.date ] ), | |||
| time: PropTypes.oneOfType( [ PropTypes.string, PropTypes.instanceOf( Date ) ] ), | |||
There was a problem hiding this comment.
@brunobasto didn't see it mentioned in the PR, is it a leftover or not? (I believe the new code is correct so I don't consider this an issue, just wondering)
There was a problem hiding this comment.
Oh, forgot to mention on the PR description, but mentioned on the commit message:
(5603609)
Fix invalid PropType error:
`Warning: Invalid argument supplied to oneOfType. Expected an array of check functions, but received undefined at index 1.`
Noticed this error when running tests.
…ise an A11Y violation in the storybook Co-authored-by: Andrea Grassi <andrea-sdl@users.noreply.github.com>
Users can control the scope directly when using <TableCell />
Description
Right now users don't have much control over the markup of the
<td>on a table. This PR allows them to build tables with more granular control. Like this:Note that we can now specify styles for the
<td>or<th>which we couldn't do before. One use case is on the Dashboard: I want the first column to have a specific width since it only contains a thumbnail.Before
After
Checklist
Steps to Test
npm run dev.<td>with its own background