#6547 add continious deletion of IOU amount on long press by dklymenk · Pull Request #6626 · Expensify/App

dklymenk · 2021-12-07T19:22:55Z

Details

This PR adds a long press handler to delete button on mobile numpad that starts continuous deletion of characters until it is released.

Fixed Issues

$ #6547

Tests / QA Steps

Navigate to a conversation
Start the Request money flow
Type some amount with several digits
Press and hold the delete button (or Backspace key on web and desktop)
The amount should be continuously deleted until the key is released

Tested On

Screenshots

Web

Mobile Web

Simulator.Screen.Recording.-.iPhone.12.-.2021-12-08.at.12.02.18.mp4

Desktop

iOS

Simulator_Screen_Recording_iPhone_12_2021_12_07_at_22_05_53.mp4

Android

6547-android.mp4

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

dklymenk · 2021-12-08T17:46:05Z

Applied the changes suggested by @parasharrajat

parasharrajat

LGTM and tests well.

All yours @iwiznia

iwiznia · 2021-12-09T16:08:46Z

+     *
+     * @param {String} key
+     */
+    handleLongPressNumberPad(key) {


Hmmm shouldn't all this logic be on the BigNumberPad component? Do we expect any BigNumberPad were we don't want long presses to work? I would think we always want this behavior, so reimplementing it on each caller sounds worse than just incorporating it to that component.

Good point, I was thinking the same but had doubts.

Basically, my point was Continues keypress event timer logic should be inside the BigNumberPad. This callback should just be doing side-effect.

So that each key reflect the same behaviour and we can choose to execute only on < for now.

But that component doesn't have any state itself, it is simply responsible to passing the callbacks to the buttons.

Or do you think we should pass some 'deleteSingleChar' prop to the number pad and call it with setInterval inside the BigNumberPad?

Yup, so you just need to trigger the passed callback as you do now. But longPress will trigger that multiple times.

Ah, I got you. There is no need for a new prop, I need to call the one that is already used for single press. 👍

@parasharrajat @iwiznia
I have updated the PR with the change you requested, please review.

parasharrajat

Do not follow StyleGuide. Please have a read of https://github.com/Expensify/App/blob/main/STYLING.md and https://github.com/Expensify/App/blob/main/STYLE.md.

dklymenk · 2021-12-09T19:35:37Z

Do not follow StyleGuide. Please have a read of https://github.com/Expensify/App/blob/main/STYLING.md and https://github.com/Expensify/App/blob/main/STYLE.md.

Do you want me to rewrite the component as class component? Before committing I have checked for usages of useState in the app and found at least one component that uses it, so it didn't seem like I am breaking the rules.

Anyway, just let me know if that's what you meant. I am slightly confused by you referencing the styling doc where this PR doesn't touch any styles whatsoever.

Thanks.

parasharrajat · 2021-12-09T19:41:05Z

Do you want me to rewrite the component as class component? Before committing I have checked for usages of useState in the app and found at least one component that uses it, so it didn't seem like I am breaking the rules.

Yeah, this should be converted to the class component. You should follow styleGuide as much as possible. Exceptions could be made but do you think that is necessary here. I am sure you read the comment on the component

App/src/components/AddressSearch.js

Line 38 in 5f154bc

    
           // Do not convert to class component! It's been tried before and presents more challenges than it's worth.

.

dklymenk · 2021-12-09T19:52:13Z

Ah, no. I missed that comment, sorry. I will try to update the BigNumberPad to use React.Component.

Thank you.

dklymenk · 2021-12-10T08:55:00Z

Updated PR with requested change: rewrite BigNumberPad as class component. Please review. @parasharrajat @iwiznia

parasharrajat · 2021-12-10T09:49:37Z

+        if (key !== '<') {
+            return;
+        }


This should not be part of BigNumberPad. This should be handled in the parent component. The only job of this method should be to call the long press callback on time intervals emitting continuous long-press.

Sorry, but I don't quite understand. There is no more long press callback in that context. I think we have agreed that this entire logic should be moved to BigNumberPad component.

I have just reread the thread above and @iwiznia clearly says "shouldn't all this logic be on the BigNumberPad component". If we are to do

if (key !== '<') { return; }

inside parent component we would need to also pass some new arg to callback to indicate that the keyPress was started by the timer. And overall moving it to parent component would mean that we'll have timers on every button and not just the <.

I agree with @dklymenk, not sure why would anyone want to handle the long presses differently. But even if at some point we do want that, we can extract the logic then.

OK, I give it a go for now. I still feel that all keys should have the same behaviour irrespective of the Key. If we only want to handle long-press for < then it should be a parent to control that.

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

iwiznia · 2021-12-10T10:34:53Z

I think this solution is good, but will let @parasharrajat merge or keep engaging on the discussion above.

parasharrajat

LGTM. we are good to merge @iwiznia .

OSBotify · 2021-12-13T19:50:54Z

✋ 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 · 2021-12-16T01:26:38Z

🚀 Deployed to staging by @iwiznia in version: 1.1.20-3 🚀

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

OSBotify · 2021-12-20T12:08:55Z

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

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

Expensify#6547 add continious deletion of IOU amount on long press

f995a22

dklymenk mentioned this pull request Dec 7, 2021

[HOLD for payment 2021-12-27] Feature Request - IOU - Holding down the delete button should continually delete the amount - Reported by: @kakajann #6547

Closed

Expensify#6547 disable selection on button long press

4489d4e

dklymenk marked this pull request as ready for review December 8, 2021 09:07

dklymenk requested a review from a team as a code owner December 8, 2021 09:07

botify requested review from iwiznia and parasharrajat December 8, 2021 09:07

MelvinBot removed the request for review from a team December 8, 2021 09:07

parasharrajat suggested changes Dec 8, 2021

View reviewed changes

dklymenk and others added 7 commits December 8, 2021 20:14

Update src/components/BigNumberPad.js

254a47b

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

Update src/pages/iou/steps/IOUAmountPage.js

c0bc433

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

Update src/pages/iou/steps/IOUAmountPage.js

484b9ea

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

Update src/pages/iou/steps/IOUAmountPage.js

c6a139a

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

fix clearLongPressTimer method name

fe8f681

add comment to handleLongPressNumberPad method

e872747

move ControllerSelection to BigNumberPad

fbebb63

parasharrajat previously approved these changes Dec 8, 2021

View reviewed changes

iwiznia reviewed Dec 9, 2021

View reviewed changes

Expensify#6547 move the logic to BigNumberPad

5f154bc

dklymenk dismissed parasharrajat’s stale review via 5f154bc December 9, 2021 19:08

parasharrajat suggested changes Dec 9, 2021

View reviewed changes

Expensify#6547 rewrite BigNumberPad as class component

065b673

parasharrajat suggested changes Dec 10, 2021

View reviewed changes

dklymenk and others added 2 commits December 10, 2021 13:18

Update src/components/BigNumberPad.js

113b3f9

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

Update src/components/BigNumberPad.js

6023a2f

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

dklymenk added 2 commits December 10, 2021 13:28

fix suggested tab character + apply suggested prop position swap

cdce9b0

apply suggested interval change to make code more understandable

4733acc

iwiznia approved these changes Dec 10, 2021

View reviewed changes

parasharrajat approved these changes Dec 10, 2021

View reviewed changes

iwiznia merged commit aae379c into Expensify:main Dec 13, 2021

OSBotify mentioned this pull request Dec 15, 2021

Deploy Checklist: New Expensify 2021-12-15 #6778

Closed

37 tasks

Uh oh!

Conversation

dklymenk commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dklymenk commented Dec 8, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

dklymenk commented Dec 9, 2021

Uh oh!

parasharrajat commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dklymenk commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dklymenk commented Dec 10, 2021

Uh oh!

Uh oh!

parasharrajat Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iwiznia commented Dec 10, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Dec 13, 2021

Uh oh!

OSBotify commented Dec 16, 2021

Uh oh!

OSBotify commented Dec 20, 2021

Uh oh!

dklymenk commented Dec 7, 2021 •

edited

Loading

parasharrajat Dec 9, 2021 •

edited

Loading

parasharrajat commented Dec 9, 2021 •

edited

Loading

dklymenk commented Dec 9, 2021 •

edited

Loading

parasharrajat Dec 10, 2021 •

edited

Loading