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(react): support react 17 #1680

Merged
merged 37 commits into from
Oct 16, 2024
Merged

feat(react): support react 17 #1680

merged 37 commits into from
Oct 16, 2024

Conversation

anastasialanz
Copy link
Contributor

@anastasialanz anastasialanz commented Sep 17, 2024

closes #1473
closes #1574
closes #1087
closes #830

This PR does a few things:

  • For the OptionsMenu and Popover component, when a user opens them, clicking on the trigger buttons will now close them instead of triggering the click outside close() method
  • Converts the OptionsMenu components to function components
  • Updates the TopBar docs so that the OptionsMenu now has a trigger to reference

Verification Steps

  • Go to the OptionsMenu component
    • Open the OptionsMenu
    • Clicking on the trigger for the OptionsMenu will close the OptionsMenu
    • Verify that the click outside handler works
  • Go to the Popover component
    • Open the Popover
    • Clicking on the trigger for the Popover will close the Popover
    • Verify that the click outside handler works
  • Go to the TopBar with menu component
    • Open the OptionsMenu inside the TopBar
    • Clicking on the trigger for the OptionsMenu will close the OptionsMenu
    • Verify that the click outside handler works
  • Go to the Modal component
    • Verify that the Modal component opens and closes with the trigger

@anastasialanz anastasialanz changed the title fix: react 17 upgrade feat: react 17 upgrade Sep 17, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1680.d15792l1n26ww3.amplifyapp.com

@anastasialanz anastasialanz changed the title feat: react 17 upgrade feat(react): support react 17 Sep 18, 2024
@anastasialanz anastasialanz self-assigned this Sep 18, 2024
@anastasialanz anastasialanz force-pushed the fix--react-17-upgrade branch from 991f913 to aaa242c Compare October 1, 2024 21:04
@anastasialanz anastasialanz force-pushed the fix--react-17-upgrade branch from fcb43d8 to cda2956 Compare October 2, 2024 16:42
@anastasialanz anastasialanz force-pushed the fix--react-17-upgrade branch from 6e50a3a to ede47b8 Compare October 3, 2024 20:03
@anastasialanz anastasialanz marked this pull request as ready for review October 8, 2024 19:11
@anastasialanz anastasialanz requested review from a team as code owners October 8, 2024 19:11
@anastasialanz anastasialanz force-pushed the fix--react-17-upgrade branch from ad1a367 to 250db1f Compare October 8, 2024 19:22
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

Everything looks pretty good, but I do think we need to address the breaking change here.

Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

Code changes LGTM!

I did a check with everything in the deploy preview and it looks like we've resolved all of the issues! There does appear to be 2 tests failing which we need to determine why they're failing before this can be merged, but otherwise great work!

docs/pages/components/TopBar.mdx Outdated Show resolved Hide resolved
@anastasialanz
Copy link
Contributor Author

anastasialanz commented Oct 16, 2024

Code changes LGTM!

I did a check with everything in the deploy preview and it looks like we've resolved all of the issues! There does appear to be 2 tests failing which we need to determine why they're failing before this can be merged, but otherwise great work!

I updated the onClose test that failed. The new code in the useEffect for OptionsMenuList was returning early and not closing because the triggerElement was the body element which contains the target. I needed to modify the test to add a trigger, focus on it, and then rerender with the OptionsMenuList open which mimics what you would see when used within another component.

@anastasialanz anastasialanz requested a review from scurker October 16, 2024 02:50
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

🎈 🎉 🎆

@scurker scurker merged commit 453f7da into develop Oct 16, 2024
8 checks passed
@scurker scurker deleted the fix--react-17-upgrade branch October 16, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants