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

[Chat] [IOU] Create IOU Amount step #1691

Closed
trjExpensify opened this issue Mar 9, 2021 · 10 comments · Fixed by #1768, #2010 or #2203
Closed

[Chat] [IOU] Create IOU Amount step #1691

trjExpensify opened this issue Mar 9, 2021 · 10 comments · Fixed by #1768, #2010 or #2203
Assignees

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Mar 9, 2021

If you haven’t already, check out our contributing guidelines for onboarding!


This issue builds on top of the existing IOU Modal, which was implemented in this PR. The IOUAMount step component has already been created (original PR) and functions as a controlled component (IOUModal manages its state).

The page allows users to select the amount they are requesting from a single user or group. To open the Modal, you can navigate to localhost:8080/#/iou/request AND localhost:8080/#/iou/split.

Mobile:
BNP
Web/Desktop:
image

Deliverables:

  1. Create the UI displayed in the above screenshot: An amount text field (not an input field), a currency symbol text field, a collection of buttons making up the BigNumberPad (mobile and mobile web only), and a button for progressing to the next step.
  2. As the Component state is managed by IOUModal, selectedCurrency and amount should be added as IOUAmount props.
  3. Add callback functions as addition props (numberPressed, backspacePressed, & currencySelected), similar to the already existing onStepComplete prop.
  4. Persist selectedCurrency(in code form: 'GBP, USD, EUR') and amount within IOUModal's state, pass this down to the IOUAMount Component via the props you just added.
  5. For now, currency will remain hardcoded as 'USD' within the IOUModal state. The currency Text component can also be set to use the currency code, instead of the currency symbol (currency will be added in a separate issue).
  6. When the function props are called, verify the validity of the input via a decimal number regex, and update the Amount Text field if valid
  7. When the currencySelected function prop is called, update the currency code within IOUModal state (for now, this prop function won't be called -- but please test it with a currency code, like USD, EUR, AUD).
  8. Enable/disable the next button depending on the validity of the amount (matching against the decimal number regex)
  9. The page title should be set to 'Request Money' or 'Split Bill' based upon the current route (example). The current solution should be replaced.

Job posting: https://www.upwork.com/jobs/~01e9946a32707d6c32

Edit: Added Web/Desktop mock

@tugbadogan
Copy link
Contributor

Hi,

I would like to work on this issue. Here is my proposal:

  1. Create the UI displayed in the above screenshot: An amount text field (not an input field), a currency symbol text field, a collection of buttons making up the BigNumberPad (mobile and mobile web only), and a button for progressing to the next step.

I will modify IOUAmountPage component and create this UI.
https://github.com/Expensify/Expensify.cash/blob/10a21d6778da60aa913f820e2c93888efb585a9e/src/pages/iou/steps/IOUAmountPage.js

I have a question. What kind of UI do you want for Web and Desktop?

  1. As the Component state is managed by IOUModal, selectedCurrency and amount should be added as IOUAmount props.

I will add these props here:
https://github.com/Expensify/Expensify.cash/blob/10a21d6778da60aa913f820e2c93888efb585a9e/src/pages/iou/steps/IOUAmountPage.js#L14-L26

  1. Add callback functions as addition props (numberPressed, backspacePressed, & currencySelected), similar to the already existing onStepComplete prop.

I will add these callback functions below this line:
https://github.com/Expensify/Expensify.cash/blob/10a21d6778da60aa913f820e2c93888efb585a9e/src/pages/iou/steps/IOUAmountPage.js#L16
And implement them in IOUModal.

  1. Persist selectedCurrency(in code form: 'GBP, USD, EUR') and amount within IOUModal's state, pass this down to the IOUAMount Component via the props you just added.

I will add selectedCurrencyand amount to IOUModal's state.
https://github.com/Expensify/Expensify.cash/blob/8025db3fd62b04f6c08dc54d907f72609f2a9695/src/pages/iou/IOUModal.js#L39-L41

  1. When the function props are called, verify the validity of the input via a decimal number regex, and update the Amount Text field if valid

Function implementation in IOUModal will validate input with regex and update the component state with new amount.

  1. When the currencySelected function prop is called, update the currency code within IOUModal state (for now, this prop function won't be called -- but please test it with a currency code, like USD, EUR, AUD).

I will update the IOUModal state with passed currency code.

  1. Enable/disable the next button depending on the validity of the amount (matching against the decimal number regex)

I will pass another boolean flag for validity of amount to enable/disable Next button.
https://github.com/Expensify/Expensify.cash/blob/10a21d6778da60aa913f820e2c93888efb585a9e/src/pages/iou/steps/IOUAmountPage.js#L21-L25

  1. The page title should be set to 'Request Money' or 'Split Bill' based upon the current route (example). The current solution should be replaced.

I will use Route tags to display corresponding page title like this:
https://github.com/Expensify/Expensify.cash/blob/621d02020e3a8d82afbc2a059535ef20458cdcf5/src/pages/iou/steps/IOUParticipantsPage.js#L36-L41

@Julesssss
Copy link
Contributor

Hey @tugbadogan, thanks again for the proposal!

I've assigned the issue to you. Please feel free to reach out if you have any questions or concerns regarding the specification.

@tugbadogan
Copy link
Contributor

Hi @Julesssss @trjExpensify

As you requested I need to implement BigNumberPad for native apps and mobile web. I found how to design separate design for native app and web/desktop. But I'm not sure how to handle Web and Mobile Web difference without adding platform specific code in the module. Do you have any example about that?

In web and desktop, amount text field is going to be an input field, isn't it?

@roryabraham
Copy link
Contributor

I found how to design separate design for native app and web/desktop. But I'm not sure how to handle Web and Mobile Web difference without adding platform specific code in the module. Do you have any example about that?

Hi @tugbadogan, what I would recommend in this case is to differentiate by screen width instead of platform. So you can wrap your component with the withWindowDimensions HOC, and then check the value of the isSmallScreenWidth prop. If true, you can assume you're dealing with a mobile device. If false, assume you're dealing with a laptop or desktop computer. That's probably sufficient for this use case in my opinion.

@AndrewGable
Copy link
Contributor

Hey @tugbadogan - Totally agree with @roryabraham here. Also here is a good example of this: https://github.com/Expensify/Expensify.cash/blob/aec6a4508b31f70c2e3c7874fd28896132466485/src/pages/signin/LoginForm/index.js#L10-L14

To answer your second question:

In web and desktop, amount text field is going to be an input field, isn't it?

Yes - But @Julesssss please confirm!

@Julesssss
Copy link
Contributor

In web and desktop, amount text field is going to be an input field, isn't it?

Hi @tugbadogan. Yeah, Web/Desktop will function as an input field. But as you can see in the mockup we would like the input background colour to match the page background colour.

Thanks @roryabraham, @AndrewGable!

@tugbadogan tugbadogan mentioned this issue Mar 14, 2021
5 tasks
@tugbadogan
Copy link
Contributor

Thank you for your help @roryabraham @AndrewGable @Julesssss, I've created the PR now. However, IOU modal view has some problems on master branch and I created an issue for those problems #1769

I tested my code with a previous version and it's working as expected. I uploaded screenshots and screen recordings to the PR.

@johnmlee101
Copy link
Contributor

johnmlee101 commented Mar 22, 2021

Hi @tugbadogan looks like a regression was introduced here:

The line introduced in your code here seems to be the cause:
https://github.com/Expensify/Expensify.cash/blob/82d9f5c793ee1f70a1ec2caa870704e7c106580e/src/components/TextInputFocusable/index.js#L113

Could you create a follow-up PR to address this?

Let me know if you have any questions about this!

Regression

Platform - version:

web - v1.0.2-91

Action Performed (reproducible steps):

  1. type out a message in the compose box
  2. move cursor back to an earlier character/space in the text
  3. type something then stop

Expected Result:

The cursor stays where you stopped

Actual Result:

The cursor jumps forward to the end of the text

Notes/Photos/Videos:

2021-03-22_09-35-40 (1)

@tugbadogan
Copy link
Contributor

Hi @johnmlee101,

Thanks for flagging this issue. In order to resolve this issue, I'm planning to add a prop to TextInputFocusable component to control cursor position (whether we should keep the cursor at the end all the time) and set this prop in IOUAmount page.

I will send the PR soon.

@tugbadogan
Copy link
Contributor

tugbadogan commented Mar 23, 2021

Hi @johnmlee101,

I fixed the problem and created a PR.
#2010

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants