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

Web/Desktop Press Enter key to go next #3977

Merged
merged 18 commits into from
Jul 15, 2021
Merged

Web/Desktop Press Enter key to go next #3977

merged 18 commits into from
Jul 15, 2021

Conversation

rushatgabhane
Copy link
Member

@rushatgabhane rushatgabhane commented Jul 11, 2021

Details

Convert the functional component Button to a class component.

Create a new prop pressOnEnter for Button.
If it's true, onPress() is called when Enter key is pressed.

This is done by adding Enter as a keyboard shortcut for onPress() using libs/KeyboardShortcut

componentDidMount() {
    return if pressOnEnter is false

      KeyboardShortcut.subscribe('Enter', () => {
          if (button not disabled or button not disabled)
              props.onPress()
      })

}

Unsubscribe on unmount.

For all the pages like request payment, add workspace etc.
Pass the prop pressOnEnter to their buttons, so that onPress() is called when Enter key is pressed.

Remove redundant event listeners for Enter from BaseModal.
And add pressOnEnter to replicate previous behaviour of ConfirmModal

Fixed Issues

$ #3785

Tests

Request payment

  1. Request payment to any user
  2. Enter amount and press Enter to go next.
  3. Verify that pressing Enter makes the payment request.

Add PayPal.me

  1. Settings -> Payments
  2. Add PayPal.me
  3. Enter user name
  4. Verify that pressing Enter adds PayPal account.

Add workspace

  1. Click on + icon.
  2. Add Workspace
  3. Enter workspace name
  4. Verify that pressing Enter adds workspace.

Split bill in group chat

  1. Go to a group chat
  2. Split bill.
  3. Enter amount, press Enter to go next.
  4. Verify that pressing Enter confirms split.

Workspace Invite Page

  1. Go to any workspace
  2. Navigate to People tab
  3. Click Invite
  4. Add the email or phone
  5. Verify that pressing Enter invites the user.

QA

Verify that mobile apps don't crash by navigating to the above mentioned pages.

Tested On

  • Web
  • Desktop

Screenshots

Web

Screencast.from.14-07-21.08_27_42.AM.+03.mp4
Screencast.from.14-07-21.08_33_19.AM.03.mp4

Desktop

@rushatgabhane rushatgabhane marked this pull request as ready for review July 14, 2021 01:24
@rushatgabhane rushatgabhane requested a review from a team as a code owner July 14, 2021 01:24
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team July 14, 2021 01:24
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Looks mostly ok. The displayed amount seems to have some style issues in both iOS and android versions.

Screenshot_1626296933
Simulator Screen Shot - iPhone 12 - 2021-07-14 at 15 10 17

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Jul 14, 2021

Looks mostly ok. The displayed amount seems to have some style issues in both iOS and android versions.

@luacmartins I locally merged with main, and this issue wasn't there.

This branch (enter-go-next) doesn't modify any styling.
This makes me believe it's a regression from previous commits, and it's fixed in the newer commits.

I think we're good to merge with main, if there isn't any other issue.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

I agree that it is probably a regression. LGTM, we just need to fix the conflict and we are good to go.

@luacmartins
Copy link
Contributor

Thanks for the fixes! One last comment that I have: pressing enter selects/deselects the first recent contact on Split Bills, is that intended behavior? Should we make it so enter moves the user to the next step if they have other users selected?

Screen.Recording.2021-07-15.at.12.53.51.PM.mov

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Jul 15, 2021

Thanks for the fixes! One last comment that I have: pressing enter selects/deselects the first recent contact on Split Bills, is that intended behavior? Should we make it so enter moves the user to the next step if they have other users selected?

Screen.Recording.2021-07-15.at.12.53.51.PM.mov

Yes, that's the intended behaviour.
The page for selecting split participants doesn't really work well with keyboard. The same is true for currency selector.

Both need to be fixed in a seperate issue because of support for arrow keys etc.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @rushatgabhane!

@luacmartins luacmartins merged commit f497ba8 into Expensify:main Jul 15, 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 in version: 1.0.78-3🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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