feat(Table, Datagrid): Added new tableTitle prop
#1541
Conversation
🦋 Changeset detectedLatest commit: 9138b7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
chris-cedrone-cengage
changed the title
| @@ -51,6 +53,7 @@ export interface DatagridRow { | |||
| * Used to allow each unique column field as a key with the row content as the value | |||
| */ | |||
| [key: string]: any; | |||
| tableTitle?: any; | |||
There was a problem hiding this comment.
I think we want users to pass a node (React.ReactNode), and maybe also a string? but I don't think we should make this as broad as any
There was a problem hiding this comment.
We also need to add a description to explain to adopters when to use this
| export const StyledCaption = styled.caption` | ||
| display: flex; | ||
| flex: 1; | ||
| `; |
There was a problem hiding this comment.
We should check with @orion-cengage what the default heading title styles should be
There was a problem hiding this comment.
Since you're already exporting it, we should define the StyledCaption in only one place and use it in both Table and Datagrid so they will always be consistent.
packages/react-magma-dom/src/components/Datagrid/Datagrid.test.js
Outdated
Show resolved
Hide resolved
silvalaura
changed the title
tableTitle prop
…se default to set amount
| @@ -278,6 +288,12 @@ export const Datagrid = React.forwardRef<HTMLTableElement, DatagridProps>( | |||
|
|
|||
| return ( | |||
| <> | |||
| {tableTitle && ( | |||
| <StyledCaption isInverse={props.isInverse} tableTitleNode={typeof tableTitle !== 'string' ? true : false} theme={theme}> | |||
There was a problem hiding this comment.
when a prop is a boolean, it's helpful to name it isXXX or hasXXX, so we could rename this to isTitleNode or something similar
There was a problem hiding this comment.
A cleaner way to write typeof tableTitle !== 'string' ? true : false is to directly use the comparison typeof tableTitle !== 'string'. This expression already evaluates to a boolean value, so there is no need to use a ternary operator to return true or false.
| @@ -51,7 +52,7 @@ const rows = [ | |||
|
|
|||
| const Template: Story<TableProps> = args => ( | |||
| <Card isInverse={args.isInverse}> | |||
| <Table {...args}> | |||
| <Table tableTitle="Basic usage"> | |||
There was a problem hiding this comment.
Do we not need ...args anymore?
There was a problem hiding this comment.
Can we add some examples where tableTitle is a node?
| export const TableTitleWithNode = args => { | ||
| return ( | ||
| <Table tableTitle={<Heading level={1} {...args}>Heading component example</Heading>}> |
There was a problem hiding this comment.
The args should be on the table, not the heading.
| <Table tableTitle={<Heading level={1} {...args}>Heading component example</Heading>}> | |
| <Table {...args} tableTitle={<Heading level={1}>Heading component example</Heading>}> |
| @@ -122,6 +128,12 @@ export const TableContainer = styled.div<{ | |||
| : props.theme.colors.focus}; | |||
| } | |||
| `; | |||
| export const StyledCaption = styled.caption<{ isInverse: boolean; tableTitleNode: boolean; }>` | |||
There was a problem hiding this comment.
By default, captions are centered aligned.
@orion-cengage are you okay with this or should we force a different alignment?
There was a problem hiding this comment.
Looking back at the original ticket...
I think it's expected for caption to be inside the table tag
<table><caption><h2>This is the title of the table</h2></caption>...
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/caption
| export const TableTitleWithNode = args => { | ||
| return ( | ||
| <Table tableTitle={<Heading level={1}>Heading component example</Heading>} {...args}> |
There was a problem hiding this comment.
Because {...args} is at the end, it's replacing tableTitle with the one that gets passed in so it reads Row colors instead of Heading component example
| @@ -51,7 +52,7 @@ const rows = [ | |||
|
|
|||
| const Template: Story<TableProps> = args => ( | |||
| <Card isInverse={args.isInverse}> | |||
| <Table {...args}> | |||
| <Table tableTitle="Basic usage" {...args}> | |||
There was a problem hiding this comment.
This isn't needed as it will always get overwritten by args as long as the Default has tableTitle
| @@ -337,18 +343,23 @@ const defaultArgs = { | |||
| }; | |||
|
|
|||
| export const Default = Template.bind({}); | |||
| Default.args = defaultArgs; | |||
| Default.args = { ...defaultArgs, tableTitle: 'Default' }; | |||
There was a problem hiding this comment.
tableTitle: 'Default' should be moved inside defaultArgs where all the default arguments are
| @@ -45,6 +46,11 @@ export interface TableProps extends React.HTMLAttributes<HTMLTableElement> { | |||
| */ | |||
| minWidth?: number; | |||
| rowCount?: number; | |||
| /** | |||
| * Title that appears above the Datagrid | |||
There was a problem hiding this comment.
I wonder if we should be clear that this will be inserted inside <caption>. Something like
- The title or caption of a table inside a
<caption>HTML element that provides the table an accessible description
(this applies to both Table and DataGrid)
silvalaura
added
needs design feedback
There was a problem hiding this comment.
Code looks good. Please do NOT merge until after the release (and @orion-cengage 's approval)
|
This was definitely not the intended result on this one, and that's probably my fault for not providing a design. I'll take a closer look when I get a bit of time freed up.
|
silvalaura
added
blocked
Issue: # 1395
What I did
tableTitleprop for bothTableandDatagridto enable the use of captions and further improve accessibility across both components.Screenshots:
Checklist
How to test