[WNMGDS-2707] Refactor tables #3028
Conversation
| @@ -40,7 +40,7 @@ const tableTemplateData = { | |||
| 'Statement adopted by the Continental Congress declaring independence from the British Empire.', | |||
| links: ( | |||
| <a href="https://billofrightsinstitute.org/founding-documents/declaration-of-independence/"> | |||
| https://billofrightsinstitute.org/founding-documents/declaration-of-independence/ | |||
| Declaration of Independence | |||
There was a problem hiding this comment.
I thought our table styles weren't responsive but discovered the issue was in the text string didn't break correctly when a naked link was used. Rather than adding styles to fix this example, I fixed the example to work with our styles.
| }, | ||
| }; | ||
| // This UI doesn't seem to work with stackable layout | ||
| // export const MultiHeaderTable: Story = { |
There was a problem hiding this comment.
Commented this out because I have questions... the multi-header table seems very different from the default story; it's almost like a native table example that also uses our components? I'm seeing a mix of things like <col> and <TableRow>... Is this more of a pattern than a component?
The other question is... now that we force the stackable layout for small breakpoints it kinda breaks this entire example. Looking at PROD and I'm not entirely sure how this pattern could work with the stacked layout.
What to do?
| @@ -43,34 +43,10 @@ describe('Table', function () { | |||
| expect(screen.getByRole('table')).toHaveClass('ds-c-table--striped'); | |||
| }); | |||
|
|
|||
| it('accepts custom id', () => { | |||
There was a problem hiding this comment.
Custom id seems to be used to tie the caption to the scrollableNotice, but since that was removed, the id served no purpose
| * Disables the warning message on development console when a responsive stackable table cell does not contain an `id` or `headers`. | ||
| * It's recommended that accessibility with screen readers is tested to ensure the stacked table meets the requirement. | ||
| */ | ||
| warningDisabled?: boolean; |
There was a problem hiding this comment.
I think I have a TS question here... TableCell has optional props (header and id) that are actually required, but only if another prop is either a td or a th. I've heard there a way to create a specific type that fits this circumstance (I think it's called a conditional type?) and I wonder if it's a better approach than having these warning props?
Something like:
interface BaseTableCellProps {
component?: 'th' | 'td';
}
interface TDProps {
header: string;
}
interface THProps {
id: string;
}
type TableCellProps = BaseTableCellProps extends 'th' ? THProps : TDProps;I'm not sure if this is a better implementation or not, so discussion would be appreciated. It just seems we have a giant bucket of props that only apply if certain other props are set and it might be cleaner to split them up?
| const classes = classNames( | ||
| 'ds-c-table', | ||
| `ds-c-table ds-c-${stackableBreakpoint}-table--stacked`, |
There was a problem hiding this comment.
In retrospect, I should just bake the stackable styles into the table class and remove the breakpoint prop... will do tomorrow morning
There was a problem hiding this comment.
Syke! After sleeping on this, I think we should keep this prop. My argument is: We don't know how many rows of data a table could have. If a table has many columns of data, the breakpoint needed for the stacked layout could be larger than that of a table with fewer cols.
| /** | ||
| * Additional classes to be added to the stacked Title element. | ||
| */ | ||
| stackedClassName?: string; |
There was a problem hiding this comment.
This prop wasn't being used?
WNMGDS-2707
To make more accessible tables, we decided to remove the option to allow for a scrollable layout and default tables to use the stacked layout for mobile.
There are still design questions:
Need to talk with a11y in the next huddle to figure out correct implementation for stackable labels.