Skip to content

fix keyboard type#6762

Merged
puneetlath merged 4 commits intoExpensify:mainfrom
thesahindia:thesahindia/ux/fix-keyboard-type
Dec 16, 2021
Merged

fix keyboard type#6762
puneetlath merged 4 commits intoExpensify:mainfrom
thesahindia:thesahindia/ux/fix-keyboard-type

Conversation

@thesahindia
Copy link
Member

Details

Fixed keyboard type on IOUAmountPage for iPad & tablet

Fixed Issues

$ #6635

Tests

QA Steps

  1. Open the app on a Android Tablet or iPad
  2. Navigate to a conversation
  3. Click on request money

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Screenshot 2021-12-15 at 2 08 06 AM

Android

Screenshot 2021-12-15 at 2 02 27 AM

@thesahindia thesahindia requested a review from a team as a code owner December 14, 2021 20:41
@MelvinBot MelvinBot requested review from parasharrajat and puneetlath and removed request for a team December 14, 2021 20:41
parasharrajat
parasharrajat previously approved these changes Dec 15, 2021
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.

cc: @puneetlath

parasharrajat
parasharrajat previously approved these changes Dec 15, 2021
src/CONST.js Outdated
NUMERIC: 'numeric',
PHONE_PAD: 'phone-pad',
NUMBER_PAD: 'number-pad',
DECIMAL_PAD: 'decimal-pad',
Copy link
Contributor

Choose a reason for hiding this comment

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

I know in the original proposal we agreed in using the decimal-pad keyboard type. However, from reading this, it looks like numeric and decimal-pad are equivalent. So could we just use the existing numeric option instead of adding this new decimal-pad option?

cc @parasharrajat

Copy link
Member

@parasharrajat parasharrajat Dec 16, 2021

Choose a reason for hiding this comment

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

Hmm... If they are the same, it does not matter unless we depend on the virtual keyboard on Android/IOS. So @thesahindia Could you retest on platforms after this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both keyboards are giving the same results. So which one should be used? @parasharrajat
Screenshot 2021-12-17 at 12 49 45 AM

Copy link
Member

Choose a reason for hiding this comment

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

So could we just use the existing numeric option instead of adding this new decimal-pad option?

From puneet's comment.

@parasharrajat
Copy link
Member

parasharrajat commented Dec 16, 2021

from the screenShot on Android, it looks like 0 is cropped. My bad that is another issue. 🗣️

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.

As Keyboard does not affect WEB| Desktop | Mobile web. it looks good to me.

Copy link
Contributor

@puneetlath puneetlath left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@puneetlath puneetlath merged commit 2a16ee5 into Expensify:main Dec 16, 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 @puneetlath in version: 1.1.21-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @sketchydroide in version: 1.1.22-0 🚀

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.

4 participants