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: menu position fix when dimensions change #3546

Conversation

mathias-berg
Copy link
Contributor

Summary

The previous fix that I did that was recently merged (#3521) worked fine when I tested it (both with the example app and with the code from this snack: https://snack.expo.dev/@fyranollfyra/menu-with-keyboard).
But when I tested the released version with my own app where I originally discovered this bug, the bug was still there when changing width of my window.

So I started digging into the code some more and made it a bit simpler and removed unnecessary listeners since we now get the window layout when opening the menu (the same time as we get the anchor and menu layout as well).

I also cleaned up some of my previous code (windowLayout.width instead of this.state.windowLayout.width) to be more in line how we use variables from state like we do for menuLayout and anchorLayout.

Test plan

Make sure the menu position changes when changing the width and height of a window or when going from landscape to portrait on phone or tablet.

Also make sure the menu doesn't get behind the keyboard (on phone or tablet) with the snack above, and that the position of the menu is correct after keyboard is dismissed.

@mathias-berg
Copy link
Contributor Author

mathias-berg commented Dec 22, 2022

@lukewalczak, my previous PR that just got merged still had some issues when changing dimensions (width). Did some changes that now work better.

@callstack-bot
Copy link

Hey @mathias-berg, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

From the code perspective changes LGTM. Tested on mobile with current examples and the one linked in the description.

@lukewalczak lukewalczak merged commit d3246a8 into callstack:main Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants