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.

Sign up for GitHub

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

Jump to bottom

fix menu placement is broken on mobile #3635

Merged
merged 4 commits into from Jun 18, 2021
Merged

fix menu placement is broken on mobile #3635

merged 4 commits into from Jun 18, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jun 17, 2021 •
edited

Details

Reverted accidentally deleted file. and refactor Code for IOS.

Fixed Issues

Fixes #3633
Fixes #3636
fixes #3631

Tests | QA Steps

  1. Open any chat on Mobile.
  2. open context menu on any message.
  3. It should open correctly from bottom to top.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

menu-m-web.mp4

Mobile Web

menu-m-web.mp4

Desktop

iOS

menu-m-web.mp4

Android

@parasharrajat parasharrajat requested a review from a team as a code owner June 17, 2021 13:52
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team June 17, 2021 13:52
@parasharrajat
Copy link
Member Author

It's a little urgent and I believe needs to go to Production.

@parasharrajat parasharrajat mentioned this pull request Jun 17, 2021
Closed
@roryabraham
Copy link
Contributor

@parasharrajat Please provide screenshots for this PR

@parasharrajat
Copy link
Member Author

Uploading

@parasharrajat parasharrajat changed the title [No QA] fix menu placement is broken on mobile fix menu placement is broken on mobile Jun 17, 2021
@parasharrajat parasharrajat mentioned this pull request Jun 17, 2021
Closed
@parasharrajat
Copy link
Member Author

@roryabraham Updated.

roryabraham
roryabraham requested changes Jun 17, 2021
View reviewed changes
src/components/Popover/index.native.js Outdated
Popover.defaultProps = defaultProps;
Popover.displayName = 'Popover';

export default withWindowDimensions(Popover);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we safely get rid of withWindowDimensions here? Seems unnecessary.

Luke9389
Luke9389 previously approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

I was the one who deleted the .native index file, but now I see that we need both because of the ReactDOM.createPortal usage. I think it was just bad timing that we did this at the same time.

@parasharrajat
Copy link
Member Author

Updated. Let's merge this quickly

@isagoico isagoico mentioned this pull request Jun 18, 2021
Merged
5 tasks
@parasharrajat
Copy link
Member Author

Let's merge this @roryabraham It's fixing deploy blockers.

@roryabraham roryabraham merged commit fb0260a into Expensify:main Jun 18, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Jun 18, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.71-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham
Copy link
Contributor

Heads up I think this may have caused a regression where the global create menu is sliding in right-to-left

@parasharrajat
Copy link
Member Author

Oh. Sending another PR. Didn't notice that part.

@parasharrajat parasharrajat mentioned this pull request Jun 18, 2021
Merged
5 tasks
@parasharrajat
Copy link
Member Author

Follow-up PR is ready here #3684. Thanks

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roryabraham roryabraham roryabraham approved these changes

@Luke9389 Luke9389 Luke9389 left review comments

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@parasharrajat @roryabraham @OSBotify @Luke9389