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

fix(DataListCheck): assign checked prop to correct attribute #7163

Merged

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Apr 1, 2022

What: Closes #6762

Based on the latest update to the DataListCheck component, I changed the attribute to which the checked prop was assigned. Having the checked attribute on the input element be either of the isChecked or checked props was causing the behavior seen in the linked issue (a console warning of changing from uncontrolled to controlled, or vice versa).

With this update, a console warning should only occur if both checked and isChecked props are passed in (which I've noted not to do in the prop descriptions). EDIT: this warning may only appear when testing in a local workspace and may not appear in the preview build.

If checked is passed in, then it's assigned to the correct defaultChecked attribute (previously being assigned to the checked attribute was the reason why the checkbox wasn't updating visually, as it wasn't updating at all I believe). If neither prop is passed in, then the component should act as a normal uncontrolled checkbox.

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 1, 2022

Copy link
Contributor

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Nice updates @thatblindgeye ! Props work as expected and updated prop descriptions look great. When both checked and isChecked are used on DataListCheck, the console warning also makes it clear that the user should only use one of these props. 👍

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit ff69b2f into patternfly:main Apr 11, 2022
jelly added a commit to jelly/cockpit-machines that referenced this pull request Sep 9, 2024
Was temporary required due to a PF bug which was fixed long ago.

patternfly/patternfly-react#7163

Related: cockpit-project/cockpit#20973
jelly added a commit to cockpit-project/cockpit-machines that referenced this pull request Sep 12, 2024
Was temporary required due to a PF bug which was fixed long ago.

patternfly/patternfly-react#7163

Related: cockpit-project/cockpit#20973
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.

DataListCheck: decide on one of checked/isChecked and make sure the checked property is propagated on the DOM
4 participants