Skip to content

Conversation

@dvail
Copy link
Contributor

@dvail dvail commented Apr 20, 2023

What: Closes #8877

Fixes the runtime error by using React's builtin isValidElement() before accessing the type property of the child value.

@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 20, 2023

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

@gitdallas will this require a codemod as well to at least give the users a warning that the type has changed?

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.

It doesn't look to me like this should break or even cause tests/snaps/selectors to fail on any existing code as if this was failing before no one would be using it this way?

I don't think we need a codemod here unless you think it's worth pointing out that it now works with non-element children. @nicolethoen

@dvail
Copy link
Contributor Author

dvail commented May 8, 2023

Agreed, the only case I can think of (which is what we hit on ACS) is if a user was conditionally rendering these children with a snippet like:

{shouldRender && <ToggleGroupItem {...props } />}

where shouldRender is almost always true, but is false in rare edge cases that aren't covered by tests. In our case this was noticed immediately but I could imagine this popping up unexpectedly for someone else.

@nicolethoen nicolethoen merged commit a1fb242 into patternfly:v5 May 9, 2023
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.

Bug - ToggleGroup - Non-ReactElement children cause a runtime render error

4 participants