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

feat(Checkbox): changed order of onChange params #8750

Merged

Conversation

wise-king-sullyman
Copy link
Contributor

What: Closes #8697

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Feb 27, 2023

@wise-king-sullyman
Copy link
Contributor Author

Just realized that I didn't need to have the explicit types everywhere like I did. I'll be pushing a change shortly to remove those where not needed to tidy things up.

Copy link
Contributor

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

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

lgtm.. just a comment

@@ -117,7 +117,7 @@ class SingleSelectInput extends React.Component {
<Checkbox
label="isDisabled"
isChecked={this.state.isDisabled}
onChange={this.toggleDisabled}
onChange={(_event, checked) => this.toggleDisabled(checked)}
Copy link
Contributor

@gitdallas gitdallas Feb 28, 2023

Choose a reason for hiding this comment

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

on some of these files where there are a ton of onChange, i think i would have updated the toggleDisabled to have the event param. the diff would then have 1 change vs a bunch, and the unused prop in 1 place vs many.

although i'm not sure if there is a standard for whether the unused param should be here or there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think each toggleDisabled here was in a different example, so changing it at the function declaration level wouldn't have reduced the number of changes needed. Could be mistaken though.

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.

Checkbox: change order of params for onChange so event is first.
5 participants