Skip to content
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

chore(table): add types, examples, and demo for onRowClick #3265

Merged
merged 2 commits into from Nov 5, 2019

Conversation

@seanforyou23
Copy link
Member

seanforyou23 commented Nov 5, 2019

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

Screen Shot 2019-11-05 at 9 19 40 AM

@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

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 5, 2019

PatternFly-React preview: https://patternfly-react-pr-3265.surge.sh

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 5, 2019

Codecov Report

Merging #3265 into master will increase coverage by 2.68%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#misc 95.45% <ø> (?)
#patternfly3 69.3% <ø> (?)
#patternfly4 64.76% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
...ernfly-4/react-table/src/components/Table/Body.tsx 75% <100%> (+2.08%) ⬆️
...patternfly-react/src/components/Spinner/Spinner.js 64.51% <0%> (-35.49%) ⬇️
...y-3/patternfly-react/src/components/Badge/index.js 62.5% <0%> (-20.84%) ⬇️
...-3/patternfly-react/src/components/Switch/index.js 62.5% <0%> (-20.84%) ⬇️
...patternfly-react/src/components/Toolbar/Toolbar.js 83.05% <0%> (-16.95%) ⬇️
...-3/patternfly-react/src/components/Button/index.js 88.23% <0%> (-11.77%) ⬇️
...ly-3/patternfly-react/src/components/Tabs/index.js 71.42% <0%> (-9.83%) ⬇️
...atternfly-react/src/components/Breadcrumb/index.js 72.22% <0%> (-9.03%) ⬇️
...ernfly-react/src/components/Wizard/WizardFooter.js 77.41% <0%> (-8.3%) ⬇️
...ponents/ApplicationLauncher/ApplicationLauncher.js 90.66% <0%> (-3.22%) ⬇️
... and 457 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5991033...1356799. Read the comment docs.

Copy link
Contributor

karelhala left a comment

Looking good! Just one question, but overall looks good.

@@ -116,7 +124,7 @@ export const TableBody = ({
className = '' as string,
children = null as React.ReactNode,
rowKey = 'id' as string,
onRowClick = (...args: any) => undefined as any,
onRowClick = undefined,

This comment has been minimized.

Copy link
@karelhala

karelhala Nov 5, 2019

Contributor

Is there any reason why you removed the empty function with undefined and if?

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 5, 2019

Author Member

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.

@karelhala

This comment has been minimized.

Copy link
Contributor

karelhala commented Nov 5, 2019

I also noticed that the default onRowClick function would fire on ever row click, even if it wasn't defined by the user

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.

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
@seanforyou23 seanforyou23 force-pushed the seanforyou23:table-rowclick branch from ed2267c to 3fb644b Nov 5, 2019
@seanforyou23

This comment has been minimized.

Copy link
Member Author

seanforyou23 commented Nov 5, 2019

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!

@seanforyou23 seanforyou23 requested a review from karelhala Nov 5, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 5, 2019

PatternFly-React preview: https://patternfly-react-pr-3265.surge.sh

@@ -37,6 +49,7 @@ class ContextBody extends React.Component<TableBodyProps, {}> {
isInput: (event.target as HTMLElement).tagName !== 'INPUT',
isButton: (event.target as HTMLElement).tagName !== 'BUTTON'
};

onRowClick(event, row, rowProps, computedData);

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Nov 5, 2019

Member

Should we test if onRowClick exists first? That way you don't need to fire a stub function?

This comment has been minimized.

Copy link
@karelhala

karelhala Nov 5, 2019

Contributor

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.

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Nov 5, 2019

Member

Yes, we use this pattern a lot with stateless components. Don't agree it would "blow UI", but don't feel strongly about it.

@tlabaj
tlabaj approved these changes Nov 5, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 2034af9 into patternfly:master Nov 5, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.