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

Copy Option Enabled for small screen devices also #5746

Merged

Conversation

Santhosh-Sellavel
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel commented Oct 10, 2021

Details

Just enabled copy email/phone number options for small screen devices.

Fixed Issues

$ #4905

Tests & QA Steps

  1. Go to any user chat.
  2. Navigate to user details by a tap on the chat header
  3. Now copy option will be shown for email/phone.

Tested On

  • Mobile Web
  • iOS
  • Android
  • Desktop
  • Web

Screenshots

Mobile Web

mWeb_01

iOS

iOS_02.mp4

Android

Android-03

Desktop

Screenshot 2021-10-11 at 11 51 13 PM

Web

Screenshot 2021-10-11 at 11 56 46 PM

@Santhosh-Sellavel Santhosh-Sellavel requested a review from a team as a code owner October 10, 2021 19:14
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team October 10, 2021 19:14
@parasharrajat
Copy link
Member

parasharrajat commented Oct 11, 2021

@Santhosh-Sellavel Could you please test it on the other remaining platforms and attach screens for those? Just to make sure they are not affected.

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Oct 11, 2021

I've tested out, It works well. As requested I'll update screenshots later (ASAP). If you are facing any issues let me know. Thanks!

cc: @parasharrajat

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Oct 11, 2021

@Santhosh-Sellavel Could you please test it on the other remaining platforms and attach screens for those? Just to make sure they are not affected.

@parasharrajat Thanks for bringing this up.

I was looking at this again. There is some minor issue with the web/desktop.

Copy button should be close to the email, but it's weirdly away from the email.

When I created this issue in staging,

Screenshot 2021-10-11 at 11 11 28 PM

Even in staging its away from email.

Screenshot 2021-10-11 at 11 07 52 PM

The difference is caused by the PR #5532

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Oct 11, 2021

@parasharrajat
We can get a confirmation from @shawnborton which is expected now, let's get it done along with this PR.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 11, 2021

Yes, I noticed that. Thanks for looking into it. Unfortunately that PR is over the regression period. we are working with the copy icon in this PR thus would you like to fix that too? I am sure that is a very minor issue.

what do you think?

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Oct 11, 2021

No issues will handle it here. @parasharrajat

@parasharrajat
Copy link
Member

Thank you. I will wait for the fix and review it again.

@shawnborton
Copy link
Contributor

Agree, copy button should be closer to the email address.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

Ready for final review after n6-hold is lifted. cc. @Luke9389

@Santhosh-Sellavel Santhosh-Sellavel changed the title [Hold N6] Copy Option Enabled for small screen devices also Copy Option Enabled for small screen devices also Oct 19, 2021
@parasharrajat
Copy link
Member

@Santhosh-Sellavel Could you please merge the main branch? There have been tremendous changes during the N6 launch. It would be great to see if we don't have any issues due to that in this PR. Thanks.

@Luke9389 Luke9389 merged commit d0c166e into Expensify:main Oct 19, 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
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.8-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants