Skip to content

Conversation

ericsoderberghp
Copy link
Contributor

What does this PR do?

Added DataTableGroupBy

Where should the reviewer start?

DataTableGroupBy.js

What testing has been done on this PR?

added a story
added a simple unit test

How should this be manually tested?

story

Do Jest tests follow these best practices?

followed other Data tests for now

Any background context you want to provide?

What are the relevant issues?

#6733

Screenshots (if appropriate)

Do the grommet docs need to be updated?

yes

Should this PR be mentioned in the release notes?

yes

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

backwards compatible

@ericsoderberghp ericsoderberghp linked an issue Apr 14, 2023 that may be closed by this pull request
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is outside the scope of this PR, but I'm wondering if we should revisit the styling for grouping in a DataTable. When I grouped by 'percent complete' in this example I felt that the grouping styling wasn't distinct enough to be helpful in visually parsing the data.

Screen Shot 2023-04-14 at 11 55 20 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. But, I'm thinking that should be a separate pull request.

Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed the propTypes.js file is missing props. Everything else looks good

@@ -0,0 +1,5 @@
let PropType = {};
if (process.env.NODE_ENV !== 'production') {
PropType = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are missing options here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added propTypes

Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ericsoderberghp ericsoderberghp merged commit 618e3b4 into master Apr 17, 2023
@ericsoderberghp ericsoderberghp deleted the feat/datatablegroup branch April 17, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data - Missing DataTableGroup component
3 participants