-
Notifications
You must be signed in to change notification settings - Fork 1k
Added DataTableGroupBy #6749
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 DataTableGroupBy #6749
Conversation
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.
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.
Agreed. But, I'm thinking that should be a separate pull request.
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.
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 = {}; |
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.
Looks like we are missing options
here
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.
added propTypes
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.
Looks good!
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.
LGTM.
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