-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Added the ability to use DataTable rowDetails in a controlled way. #6571
base: master
Are you sure you want to change the base?
Conversation
This would be super useful for us! |
Thanks for the PR! I'm still familiarizing myself with the logic, but here is some quick feedback on variable naming. I think it might be more aligned with the terms and types we use in
|
93a67a9
to
5a6ec98
Compare
…feedback, renaming property names to `expanded`, `expandable` and `onClickExpand`. And a line length fix to allow a commit.
It's taken me some(?) time to get around to fixing this, I've updated the PR with the requested changes, and updated from upstream/master. I'm a bit eager to get this functionality so a renewed review would be appreciated @taysea & @jcfilben . |
…feedback, renaming property names to `expanded`, `expandable` and `onClickExpand`. And a line length fix to allow a commit.
…able-controlled # Conflicts: # src/js/components/DataTable/Body.js
Added a PR for documentation as well: grommet/grommet-site#484 |
Hey @aheintz sorry for the delay on this one. Thank you for contributing, I think this PR is a valuable addition to Grommet. I'll try and get this reviewed early next week |
expanded: (row: TRowType) => boolean; | ||
expandable: (row: TRowType) => boolean; | ||
onClickExpand: (row: TRowType) => void; |
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.
expanded
, expandable
, and onClickExpand
should all be optional. Also in this comment @taysea mentioned changing expanded
and expandable
to be arrays of string or numbers where the items in the arrays are primary keys of rows.
<ExpanderCell | ||
context={isExpanded ? 'groupHeader' : 'body'} | ||
expanded={isExpanded} | ||
disabled={!rowExpandable} |
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.
disabled={!rowExpandable} | |
expandable={rowExpandable} |
Can we change this prop name to expandable
? I think the semantic html meaning of 'disabled' doesn't align with what we are doing here.
@@ -53,15 +53,15 @@ const ExpanderControl = ({ context, expanded, onToggle, pad, ...rest }) => { | |||
return content; | |||
}; | |||
|
|||
const ExpanderCell = ({ background, border, context, ...rest }) => ( | |||
const ExpanderCell = ({ background, border, context, disabled, ...rest }) => ( |
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.
const ExpanderCell = ({ background, border, context, disabled, ...rest }) => ( | |
const ExpanderCell = ({ background, border, context, expandable, ...rest }) => ( |
<TableCell | ||
background={background} | ||
border={border} | ||
size="xxsmall" | ||
plain="noPad" | ||
verticalAlign={context === 'groupEnd' ? 'bottom' : 'top'} | ||
> | ||
<ExpanderControl context={context} {...rest} /> | ||
{!disabled && <ExpanderControl context={context} {...rest} />} |
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.
{!disabled && <ExpanderControl context={context} {...rest} />} | |
{expandable && <ExpanderControl context={context} {...rest} />} |
Can we controlled the rowDetail props show expand icon on all the row. i want to show only to specific row. Can we achieve with current gromment DataTable. I think the above PR is for to achieve that functionality, when we can expect this in the grommet release |
Hey @kabincs the Grommet DataTable currently does not support having rowDetails only on specific rows. This PR would enable this functionality. Unfortunately I don't have a timeline for when this will be added to Grommet. |
What does this PR do?
Extended
rowDetails
property to be either a function or a shape that may contain four functions,render
,isExpanded
,isExpandable
andexpandClick
.render
is the equivalent of the current functionrowDetails
rendering the row details.isExpanded
is the function which determines if the row details should be expandedisExpandable
is a function which allows certain rows to be expandable or not (i.e. not expandable means no expand button or expand/collapse events)expandClick
is a function which is fired when the expand/collapse button is clicked. All of these functions are called with the entire row object fromdata
.render
is mandatory.expandClick
can be handled usingonClickRow
or similar, in the absence ofisExpandable
the default is that the row is expandable and the absence ofisExpanded
falls back to the non-controlled behaviour as it is now.Storybook example is provided.
Where should the reviewer start?
I think the "RowDetails Controlled" story in
src/js/components/DataTable/stories/RowDetailsControlled.js
is a good place to start. The majority of the work is inBody.js
What testing has been done on this PR?
Added test to
DataTable-test.js
to verify the functionality. Existing tests for DataTable are untouched. Snapshots are updated.How should this be manually tested?
Storybook is a good start, play around with that code according to the description above.
Do Jest tests follow these best practices?
Shit, no. Sorry. I've followed the pattern in the existing tests. I can look into updating the tests based on this.
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
We have a need for a way to control which rows details are open (only allowing one row open is certainly one case)
What are the relevant issues?
This PR resolves the second part of the issue(s) mentioned in this issue: #6344 This PR together with PR-6570 solves that issue.
Screenshots (if appropriate)
N/A
Do the grommet docs need to be updated?
Yes. The DataTable docs need updates to match the changing properties for DataTable.rowDetails.
Should this PR be mentioned in the release notes?
Possibly, I don't know if the change is "big" enough.
Is this change backwards compatible or is it a breaking change?
The change is fully backwards compatible (all previous tests for rowDetails are untouched as well as storybook).