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

Wrapped forgot link to content #5592

Merged
merged 3 commits into from
Oct 19, 2021
Merged

Conversation

akshayasalvi
Copy link
Contributor

@mountiny Feel free to review whenever the n6-hold is lifted :) I am keeping it in the draft right now.

Details

  • Wrap forgot link width to content instead of full width.

Fixed Issues

$ #5355

Tests

  1. Tested the link clickability in all platforms

QA Steps

  1. Go to login and enter an existing email address
  2. In the password page, try to click outside the forgot link in the same row.
  3. It shouldn't allow you to click except for the width of the forgot? textlink.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-forgot-link.mov

Mobile Web

mweb-forgot-link.mov

Desktop

desktop-forgot-link.mov

iOS

ios-forgot-link.mov

Android

android-forgot-link.mov

@mountiny mountiny self-requested a review September 30, 2021 09:34
@mountiny
Copy link
Contributor

@akshayasalvi Great Job, will look into this in coming days!

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Simple change, good job @akshayasalvi, thank you for this. We will wait for the hold to be lifted, sorry for extension, there should be bonus included as a compensation for the time you wait.

@mountiny
Copy link
Contributor

mountiny commented Oct 4, 2021

Feel free to make this ready for a review.

@akshayasalvi
Copy link
Contributor Author

Feel free to make this ready for a review.

Is the n6-hold lifted?

@akshayasalvi akshayasalvi marked this pull request as ready for review October 4, 2021 11:20
@akshayasalvi akshayasalvi requested a review from a team as a code owner October 4, 2021 11:20
@botify botify requested a review from mountiny October 4, 2021 11:20
@MelvinBot MelvinBot removed the request for review from a team October 4, 2021 11:20
@akshayasalvi
Copy link
Contributor Author

Simple change, good job @akshayasalvi, thank you for this. We will wait for the hold to be lifted, sorry for extension, there should be bonus included as a compensation for the time you wait.

Just read this. Thank you :) Appreciate your kind words.

@mountiny mountiny changed the title Wrapped forgot link to content [HOLD] Wrapped forgot link to content Oct 4, 2021
@mountiny mountiny changed the title [HOLD] Wrapped forgot link to content Wrapped forgot link to content Oct 19, 2021
@mountiny
Copy link
Contributor

Hi @akshayasalvi, The n6-hold was lifted so we can go ahead with this one.

Although this PR is quite small and simple, I would prefer if we could always merge main branch to any of the n6-hold PRs and re-test. I doubt there will be any regression here, but better safe than sorry.

During the N6 rush, we have merged a lot of small (and some bigger) polish PRs internally and externally, so it is possible there will be some regressions and better to catch it now than in staging (or in production 😬 ).

@mountiny mountiny self-requested a review October 19, 2021 11:27
@akshayasalvi
Copy link
Contributor Author

@mountiny Makes sense, I'll push in a while

@akshayasalvi
Copy link
Contributor Author

@mountiny Merged main and tested again

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

LGTM! @akshayasalvi Great work on this one and thank you very much for your patience with the n6-hold.

@mountiny mountiny merged commit 546e830 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 @mountiny 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.

3 participants