-
Notifications
You must be signed in to change notification settings - Fork 357
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
Table type adjustments #2248
Table type adjustments #2248
Conversation
PatternFly-React preview: https://2248-pr-patternfly-react-patternfly.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #2248 +/- ##
==========================================
- Coverage 79.89% 79.88% -0.01%
==========================================
Files 669 668 -1
Lines 8529 8520 -9
Branches 734 731 -3
==========================================
- Hits 6814 6806 -8
Misses 1362 1362
+ Partials 353 352 -1
Continue to review full report at Codecov.
|
No issues from me and thanks for taking time to look into this. I think any good improvements on this will be helpful for #1950 later. 👍 |
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.
I like your approach with providing further information to anyone implementing new ransform function! We should also help consumers with cellTransforms
, columnTransforms
and formatters
because these are essentatially the same bu effect different part of table.
packages/patternfly-4/react-table/src/components/Table/Table.d.ts
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-table/src/components/Table/Table.d.ts
Outdated
Show resolved
Hide resolved
1742c25
to
99e32f6
Compare
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.
Looking good now! Thank you.
packages/patternfly-4/react-table/src/components/Table/index.d.ts
Outdated
Show resolved
Hide resolved
a98a660
to
047c5cc
Compare
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 really good! Thank you.
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: This PR updates the type used in react-table to be a little more explicit. Also updates the example code in the readme, following along as-is in a TypeScript application leads to the following error
I took a stab at using better types for
transforms
, what do people think about this new type? I'd like to use something more specific thanArray<Function>
if possible as this doesn't inform of what the inputs and outputs should or can be. Might be good to wait until this is converted fully to TS to address the others, but I'm curious what others think.