search

patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
762 stars 350 forks source link

DropdownToggleCheckbox causes React controlled-state warning when moving in or out of indeterminate state #4213

Closed mturley closed 3 years ago

mturley commented 4 years ago

Describe the issue. What is the expected and unexpected behavior?

Our Checkbox and DropdownToggleCheckbox components both support passing a null value for the isChecked prop to display an indeterminate state (line through the box, not checked or unchecked). For the DropdownToggleCheckbox (and not the Checkbox), when that prop changes from false to null, React throws this warning:

A component is changing a controlled input of type checkbox to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component.

Comparing the source for Checkbox and DropdownToggleCheckbox, the difference appears to be this line:

checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked;

So, we need the checked attribute on the underlying <input> to always be a boolean even when isChecked is null, since the null value just drives the indeterminate property via a ref callback.

I think the real fix for this is just to remove the native <input> from DropdownToggleCheckbox and instead reuse Checkbox in there (I think the dropdown one predated the generic one).

Please provide the steps to reproduce. Feel free to link CodeSandbox or another tool.

Use this CodeSandbox (thanks @redallen): https://codesandbox.io/s/dazzling-hill-wzf1j If you uncheck all the child checkboxes, you'll see the parent checkbox switch from indeterminate to unchecked, and the error will appear in the console.

Is this a bug or enhancement? If this issue is a bug, is this issue blocking you or is there a work-around? Bug. It causes a silent warning, but as long as the behavior still works as intended it's not a blocker.

What is your product and what release version are you targeting? This is for @AjayJagan in Kogito. This is the PR where he is seeing the issue: https://github.com/kiegroup/kogito-apps/pull/209. Not sure what release version he is targeting.

cristianonicolai commented 4 years ago

@mturley Kogito currently uses this set of versions: https://github.com/kiegroup/kogito-apps/blob/master/packages/management-console/package.json#L39 from 2020.04 release. Thanks

AjayJagan commented 4 years ago

Hey Cristiano , Yes I think its ok to merge the PR.

On Fri, 8 May 2020 at 8:30 AM, Cristiano Nicolai notifications@github.com wrote:

@mturley https://github.com/mturley Kogito currently uses this set of versions: https://github.com/kiegroup/kogito-apps/blob/master/packages/management-console/package.json#L39 from 2020.04 release. Thanks

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/patternfly/patternfly-react/issues/4213#issuecomment-625601412, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIY6IRURBGXTTD742GHIGLTRQNX7RANCNFSM4M3QRSJA .

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

mturley commented 3 years ago

This was fixed by #4844