-
Notifications
You must be signed in to change notification settings - Fork 371
Switch to createRoot
API for rendering
#8027
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
Conversation
Preview: https://patternfly-react-pr-8027.surge.sh A11y report: https://patternfly-react-pr-8027-a11y.surge.sh |
@jonkoops looks like for some reason this change causes an error to pop up in the integration tests for the menu component, do you have any guess as to why that might be? |
@wise-king-sullyman It might have something to do with the fact that under Strict Mode component effects will get triggered twice. This is done during development to ensure that effects are cleaned up properly. However, that should not impact a production build. So I am not entirely sure what is going on here. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Bumping this for stalebot |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
bump |
I'm still not sure what's going on with the failing integration test here. @nicolethoen do you have any ideas? Also would this need to be updating to target the v5 branch now? |
@wise-king-sullyman when opting in to the new root API it also enables the new concurrent renderer (see React Docs). There must be something in PatternFly that is not compatible with this new renderer. I am seeing the same in our application when the new root API is used. I have yet to find out why though, as there is no error logged to indicate anything is wrong. |
yes this should be updated to the v5 branch. I'll have to investigate further to see if I have any ideas. The last time I looked at this PR I couldn't identify the problem |
8af15a0
to
321e570
Compare
I went ahead and changed the target branch to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonkoops it does not like the "click" in this test.
cy.get('#edit').click(); |
The flyout menu item is not clickable. The flyout menu renders on mouseover.
The test case in not testing anything. You can disable it and that should fix your PR so we can merge it.
321e570
to
1e31e05
Compare
@tlabaj thanks for the tip, looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
Your changes have been released in:
Thanks for your contribution! 🎉 |
Works towards closing #7142. More information can be found in the React 18 migration guide.