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

Clean up IOUAmount page #2723

Merged
merged 22 commits into from
May 18, 2021
Merged

Clean up IOUAmount page #2723

merged 22 commits into from
May 18, 2021

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented May 6, 2021

Details

  1. Removes the ActivityIndicator
  2. Autofocuses on input on web
  3. Remember the amount entered when we return to the page from later steps

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/163041

Tests / QA

  1. Go to someone's chat and click the + button and hit 'Request Money'

  2. (On Web/Desktop), make sure the amount field is focused on (entering any text automatically adds it and the cursor is blinking)
    Screen Shot 2021-05-06 at 2 08 43 PM

  3. (On Web/Desktop) Enter any amount and hit the 'Enter' button on your keyboard. Make sure that it takes you to the next screen.

  4. (Other platforms) Enter in any amount and hit next

  5. Go back and ensure that you still see the amount you entered before

Screen Shot 2021-05-06 at 1 58 32 PM

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-05-06 at 2 34 45 PM

Mobile Web

Screen Shot 2021-05-06 at 2 53 04 PM

Desktop

Screen Shot 2021-05-06 at 2 35 56 PM

iOS

Screen Shot 2021-05-06 at 2 40 10 PM

Android

Screen Shot 2021-05-06 at 2 48 43 PM

@thienlnam thienlnam self-assigned this May 6, 2021
@thienlnam thienlnam marked this pull request as ready for review May 6, 2021 21:54
@thienlnam thienlnam requested a review from a team as a code owner May 6, 2021 21:54
@thienlnam thienlnam changed the title [WIP] Clean up IOUAmount page Clean up IOUAmount page May 6, 2021
@MelvinBot MelvinBot requested review from stitesExpensify and removed request for a team May 6, 2021 21:55
@thienlnam thienlnam changed the title Clean up IOUAmount page [HOLD] Clean up IOUAmount page May 7, 2021
@thienlnam
Copy link
Contributor Author

Got a request in the original issue to add a feature: https://github.com//Expensify/issues/163041#issuecomment-834285328

Putting on hold while I take care of that

@thienlnam thienlnam changed the title [HOLD] Clean up IOUAmount page Clean up IOUAmount page May 7, 2021
@thienlnam
Copy link
Contributor Author

Alright, ready to be reviewed! @stitesExpensify

@thienlnam
Copy link
Contributor Author

Brandon is OOO today, and since it's a daily I'm going to trigger auto-assigner again

@thienlnam thienlnam requested a review from a team May 10, 2021 16:14
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team May 10, 2021 16:14
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

I think we should avoid the setTimeout call if possible

@thienlnam thienlnam requested a review from Jag96 May 11, 2021 18:31
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Looking good, just one comment and one question!

@thienlnam
Copy link
Contributor Author

thienlnam commented May 12, 2021

Thanks for the comments, updated! I was also able to find a cleaner way to handle the focus cc @Jag96 @jasperhuangg

jasperhuangg
jasperhuangg previously approved these changes May 13, 2021
Copy link
Contributor

@jasperhuangg jasperhuangg 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!

@thienlnam
Copy link
Contributor Author

Fixed merge conflicts!

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Getting a crash on iOS because document doesn't exist in the mobile context:

@thienlnam
Copy link
Contributor Author

Removed the accidental podfile change 👍

Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Hm, I don't know where I said this before, I swear I said it, but can't find it now :(

I'm not a fan of these files being split into index/index.native like they are currently. This will be the first time that an entire page is split between platforms, and it really creates a lot of repeated code. I think a solution should be found that only abstracts the absolute least amount of logic in the platform-specific files as possible. This will decrease the cost to maintain these files in the future as it's not very clear what logic is different between the two files.

@thienlnam
Copy link
Contributor Author

I agree with you on all those points, the only logic difference in the files was the keydown event handler for Web/Desktop, which was having document is undefined errors on mobile. I thought the way to handle that would be to split them into different files but it actually turns out there is a KeyboardShortcut component that handles precisely this problem so thanks for bringing that up 👍

@thienlnam thienlnam requested a review from tgolen May 17, 2021 19:49
tgolen
tgolen previously approved these changes May 17, 2021
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Ah, that looks great! Thank you

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

I'm seeing another crash on iOS after step 4 Enter in any amount and hit next. Do you see the same @thienlnam?

TypeError: undefined is not an object (evaluating 'option.participantsList.length')
This error is located at:
    in OptionRow
    in OptionRow (at OptionsList.js:160)

@thienlnam
Copy link
Contributor Author

@Jag96 I'm seeing those same errors but they're happening on prod and unrelated to these changes #2969. Just updated back to those block comments

@thienlnam thienlnam requested a review from Jag96 May 17, 2021 22:52
@thienlnam
Copy link
Contributor Author

Going to self merge since we got 2 approvals

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