chore(table): add types, examples, and demo for onRowClick - #3265
Conversation
Sorry, something went wrong.
|
PatternFly-React preview: https://patternfly-react-pr-3265.surge.sh |
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #3265 +/- ##
==========================================
+ Coverage 64.74% 67.43% +2.68%
==========================================
Files 481 892 +411
Lines 11702 24869 +13167
Branches 2141 2140 -1
==========================================
+ Hits 7577 16771 +9194
- Misses 3119 7094 +3975
+ Partials 1006 1004 -2
Continue to review full report at Codecov.
|
Sorry, something went wrong.
karelhala
left a comment
There was a problem hiding this comment.
Looking good! Just one question, but overall looks good.
Sorry, something went wrong.
| children = null as React.ReactNode, | ||
| rowKey = 'id' as string, | ||
| onRowClick = (...args: any) => undefined as any, | ||
| onRowClick = undefined, |
There was a problem hiding this comment.
Is there any reason why you removed the empty function with undefined and if?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, I suppose it was two-fold. I replaced the default function because it executed every time a row was clicked, even if the table doesn't specify an onRowClick prop, which kinda looked like an error. Having two instances of any along with the type assertion undefined as any in the same function signature made me think it was just temporary implementation details that could stand to be cleaned up.
Sorry, something went wrong.
I don't think that it's a bad behavior, the function that is called is just an empty function. If we change this to undefined we have to check for it to be defined and that adds to a complexity of this already really complex component. |
Sorry, something went wrong.
provide type for onRowClick handler add onRowClick example to docs add onRowClick test for table add on row click demo in react-integration clean up customRowWrapper demo/example only fire onRowClick when it is provided by user
|
Thx @karelhala - I added the default function back and removed the check. I still feel kinda weird about firing stub function handlers like this as it could lead to performance issues if the pattern is repeated across many features of many components. I did take the time to add a type for the handler, though, so at least now it's a little more type-safe. Lemme know if you find anything else! |
Sorry, something went wrong.
|
PatternFly-React preview: https://patternfly-react-pr-3265.surge.sh |
Sorry, something went wrong.
| isButton: (event.target as HTMLElement).tagName !== 'BUTTON' | ||
| }; | ||
|
|
||
| onRowClick(event, row, rowProps, computedData); |
There was a problem hiding this comment.
Should we test if onRowClick exists first? That way you don't need to fire a stub function?
Sorry, something went wrong.
There was a problem hiding this comment.
That's what was in the previous version and it's not really that good. Firing empty function is nothig compared to checking if function exists. The undefined instead of empty function might hurt us in future when we forgot to check if this function is defined and blow UI.
Overall it boils down to be consistent and since almost every other component is using either () => undefined or Function() instead of checking before call I think empty function is better.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, we use this pattern a lot with stateless components. Don't agree it would "blow UI", but don't feel strongly about it.
Sorry, something went wrong.
tlabaj
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
What: This PR adds an example of how to use the onRowClick prop, and specifies more specific types for this handler function. It also adds a demo in react-integration and tests. I also noticed that the default onRowClick function would fire on ever row click, even if it wasn't defined by the user, so I added a check so that it only fires if a handler is provided by the user.
Additional issues: #3263
@KKoukiou FYI
add IComputedData type
provide type for onRowClick handler
add onRowClick example to docs
add onRowClick test for table
add on row click demo in react-integration
clean up customRowWrapper demo/example
only fire onRowClick when it is provided by user