Added the ability to use DataTable rowDetails in a controlled way. #6571

aheintz · 2023-01-10T18:01:53Z

What does this PR do?

Extended rowDetails property to be either a function or a shape that may contain four functions, render, isExpanded, isExpandable and expandClick.

render is the equivalent of the current function rowDetails rendering the row details. isExpanded is the function which determines if the row details should be expanded isExpandable 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 from data.

render is mandatory. expandClick can be handled using onClickRow or similar, in the absence of isExpandable the default is that the row is expandable and the absence of isExpanded 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 insrc/js/components/DataTable/stories/RowDetailsControlled.js is a good place to start. The majority of the work is in Body.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.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
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).

skuroyama-AoPS · 2023-03-09T19:36:35Z

This would be super useful for us!

taysea · 2023-05-25T20:04:26Z

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 groupBy (https://v2.grommet.io/datatable#groupBy):

isExpanded --> expanded: (string | number)[] (where this is the primary key of the expanded rows)
isExpandable --> expandable: (string | number)[] (where this is the primary key of the expandable rows)
expandClick --> onClickExpand or onExpand: function

…feedback, renaming property names to `expanded`, `expandable` and `onClickExpand`. And a line length fix to allow a commit.

aheintz · 2023-07-02T14:16:52Z

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 groupBy (https://v2.grommet.io/datatable#groupBy):

isExpanded --> expanded: (string | number)[] (where this is the primary key of the expanded rows)

isExpandable --> expandable: (string | number)[] (where this is the primary key of the expandable rows)

expandClick --> onClickExpand or onExpand: function

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

…6571

aheintz · 2023-07-02T16:44:41Z

Added a PR for documentation as well: grommet/grommet-site#484

aheintz · 2023-08-16T11:38:04Z

Is it on the agenda to accept this PR anytime soon @jcfilben / @taysea , or is there anything you would like to see changed? In my opinion, it is a necessary feature for an expandable data table.

aheintz · 2023-10-15T11:34:38Z

@jcfilben , @taysea is there any chance to get this approved? Feels a bit sad to spend time to try to contribute to an open source project and it just goes into the void. Provide additional feedback, approve it or if you don't like it, close it.

jcfilben · 2023-10-21T16:23:35Z

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

jcfilben · 2023-10-23T20:17:48Z

src/js/components/DataTable/index.d.ts

+        expanded: (row: TRowType) => boolean;
+        expandable: (row: TRowType) => boolean;
+        onClickExpand: (row: TRowType) => void;


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.

jcfilben · 2023-10-23T20:21:25Z

src/js/components/DataTable/Body.js

+            <ExpanderCell
+              context={isExpanded ? 'groupHeader' : 'body'}
+              expanded={isExpanded}
+              disabled={!rowExpandable}


Suggested change

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.

jcfilben · 2023-10-23T20:21:53Z

src/js/components/DataTable/ExpanderCell.js

@@ -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 }) => (


Suggested change

const ExpanderCell = ({ background, border, context, disabled, ...rest }) => (

const ExpanderCell = ({ background, border, context, expandable, ...rest }) => (

jcfilben · 2023-10-23T20:22:08Z

src/js/components/DataTable/ExpanderCell.js

  <TableCell
    background={background}
    border={border}
    size="xxsmall"
    plain="noPad"
    verticalAlign={context === 'groupEnd' ? 'bottom' : 'top'}
  >
-    <ExpanderControl context={context} {...rest} />
+    {!disabled && <ExpanderControl context={context} {...rest} />}


Suggested change

{!disabled && <ExpanderControl context={context} {...rest} />}

{expandable && <ExpanderControl context={context} {...rest} />}

kabincs · 2023-12-14T05:00:15Z

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

jcfilben · 2023-12-19T00:29:04Z

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.

jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Mar 29, 2023

taysea added the waiting Awaiting response to latest comments label Jun 8, 2023

jcfilben removed the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jun 22, 2023

aheintz closed this Jul 2, 2023

aheintz force-pushed the datatable-controlled branch 2 times, most recently from 93a67a9 to 5a6ec98 Compare July 2, 2023 13:34

Updated from upstream/master and implemented changes according to PR …

752db72

…feedback, renaming property names to `expanded`, `expandable` and `onClickExpand`. And a line length fix to allow a commit.

aheintz reopened this Jul 2, 2023

aheintz added 2 commits July 2, 2023 16:22

Updated from upstream/master and implemented changes according to PR …

3048d7c

…feedback, renaming property names to `expanded`, `expandable` and `onClickExpand`. And a line length fix to allow a commit.

Merge remote-tracking branch 'origin/datatable-controlled' into datat…

7bd26bc

…able-controlled # Conflicts: # src/js/components/DataTable/Body.js

aheintz added a commit to aheintz/grommet-site that referenced this pull request Jul 2, 2023

Documentation for controlled datatable rowdetails PR grommet/grommet#…

31dfe40

…6571

aheintz mentioned this pull request Jul 2, 2023

Documentation for controlled datatable rowdetails grommet/grommet-site#484

Open

jcfilben added PRty Biweekly PR reviews by grommet team with hoping of closing such PRs and removed waiting Awaiting response to latest comments labels Jul 6, 2023

aheintz added 2 commits July 11, 2023 07:58

Merge branch 'master' into datatable-controlled

b18fd9a

Merge branch 'master' into datatable-controlled

a7b6ca9

jcfilben reviewed Oct 23, 2023

View reviewed changes

jcfilben added waiting Awaiting response to latest comments and removed PRty Biweekly PR reviews by grommet team with hoping of closing such PRs labels Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to use DataTable rowDetails in a controlled way. #6571

Added the ability to use DataTable rowDetails in a controlled way. #6571

aheintz commented Jan 10, 2023

skuroyama-AoPS commented Mar 9, 2023

taysea commented May 25, 2023 •

edited

aheintz commented Jul 2, 2023

aheintz commented Jul 2, 2023

aheintz commented Aug 16, 2023

aheintz commented Oct 15, 2023

jcfilben commented Oct 21, 2023

jcfilben Oct 23, 2023

jcfilben Oct 23, 2023

jcfilben Oct 23, 2023

jcfilben Oct 23, 2023

kabincs commented Dec 14, 2023

jcfilben commented Dec 19, 2023

	const ExpanderCell = ({ background, border, context, disabled, ...rest }) => (
	const ExpanderCell = ({ background, border, context, expandable, ...rest }) => (

	{!disabled && <ExpanderControl context={context} {...rest} />}
	{expandable && <ExpanderControl context={context} {...rest} />}

Added the ability to use DataTable rowDetails in a controlled way. #6571

Are you sure you want to change the base?

Added the ability to use DataTable rowDetails in a controlled way. #6571

Conversation

aheintz commented Jan 10, 2023

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

skuroyama-AoPS commented Mar 9, 2023

taysea commented May 25, 2023 • edited

aheintz commented Jul 2, 2023

aheintz commented Jul 2, 2023

aheintz commented Aug 16, 2023

aheintz commented Oct 15, 2023

jcfilben commented Oct 21, 2023

jcfilben Oct 23, 2023

Choose a reason for hiding this comment

jcfilben Oct 23, 2023

Choose a reason for hiding this comment

jcfilben Oct 23, 2023

Choose a reason for hiding this comment

jcfilben Oct 23, 2023

Choose a reason for hiding this comment

kabincs commented Dec 14, 2023

jcfilben commented Dec 19, 2023

taysea commented May 25, 2023 •

edited