Fix Workspace Rate Unit Form Flow #34849

shubham1206agra · 2024-01-20T12:18:55Z

Details

Fixed Issues

$ #29533
PROPOSAL: #29533 (comment)

Tests

Launch Expensify.
Navigate to Settings > Preferences and change the Language to Spanish
Switch to a workspace.
Go to Settings from Bottom Tab > Workspaces (Espacio de trabajo) > Reimbursements (Reembolsos) > Track distance (Medir distancia) and click on the push to page item containing the current rate.
Enter any rate with a decimal separator.

Verify that no errors appear in the JS console

Offline tests

Same as Tests

QA Steps

Same as Tests

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-02-11.at.5.42.51.PM.mov

Android: mWeb Chrome

Screen.Recording.2024-02-11.at.5.03.31.PM.mov

iOS: Native

Screen.Recording.2024-02-11.at.5.16.25.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-02-11.at.4.56.19.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-02-11.at.4.51.45.PM.mov

MacOS: Desktop

Screen.Recording.2024-02-11.at.5.08.48.PM.mov

melvin-bot · 2024-01-20T12:18:58Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

shubham1206agra · 2024-01-21T18:08:07Z

Hello @akinwale

I have almost completed the workspace rate unit form. If you want, you can start preliminary review. These are the screen made now (It has minor visual changes, otherwise evrything is completed).

I will most probably open this for review tomorrow.

akinwale

Done with a preliminary review. Looks good so far, but a number of changes required.

akinwale · 2024-01-22T12:27:54Z

src/ROUTES.ts

+        route: 'workspace/:policyID/rateandunit/unit',
+        getRoute: (policyID: string, unit?: string, rate?: string) =>
+            // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
+            `workspace/${policyID}/rateandunit/unit${unit || rate ? '?' : ''}${unit ? `unit=${unit}` : ''}${unit && rate ? '&' : ''}${rate ? `rate=${rate}` : ''}` as const,


This entire block is confusing. Is there a way to simplify the parameters based on what the route actually ends with? This way, there is no need to conditionally check if it's rate or unit that is set.

For instance, for the WORKSPACE_RATE_AND_UNIT_RATE route, just return workspace/${policyID}/rateandunit/rate, and then load the rate value from Onyx.

Am I allowed to create a new Onyx key for this?

I reckon that's fine since we're moving away from a form to individual push-to-page screens. @tgolen Any thoughts on this?

Adding new Onyx keys usually needs to be coordinated with the backend to ensure the new key is being populated properly. I might be missing some context for the rest of the question.

What Onyx key are you wanting to add and what does that have to do with the suggestion here (which I agree with) to clean up the routes?

No it will be a temporary key. No need to sync it with backend.

Please answer my question:

What Onyx key are you wanting to add and what does that have to do with the suggestion here?

What Onyx key are you wanting to add and what does that have to do with the suggestion here?

I haven't thought of a name yet, but the Onyx key will behave similar to Task key, whose function would be to hold the values temporarily while we are editing the RateUnit form.

@akinwale Can you give another pass at the PR?

src/libs/Navigation/types.ts

src/pages/workspace/reimburse/WorkspaceRateAndUnitPage/UnitPage.tsx

src/pages/workspace/reimburse/WorkspaceRateAndUnitPage/RatePage.tsx

…space-rate-unit-flow

shubham1206agra · 2024-02-03T13:53:02Z

@tgolen I noticed an old code in BNP, which we don't use at all now.

The PR that introduced this #6547.
Its function was to give continuous back at long press, but now even Manual Request doesn't seem to use this, and it looks like we tried to stop on single button press only at long press. See #30082

We can do 2 things about this:

Remove long press logic completely (This will make long press do nothing at all)
On long press, we clear the input for back button, and single press for other buttons (This one I prefer more).

The reason for this question is that I am encountering a bug where long press leads to weird cursor behavior.

cc @akinwale

dubielzyk-expensify · 2024-02-09T01:19:31Z

Just from an initial look it looks like the keypad spacing is a bit off. See correct measurements on the right here:

I also think we should probably say "Track distance" in the header and remove the : Rate as that feels a bit off to me.

akinwale · 2024-02-09T01:24:15Z

I also think we should probably say "Track distance" in the header and remove the : Rate as that feels a bit off to me.

This is intended to be the title when updating the rate or unit. I think we could use just "Rate" or "Unit" respectively if we're getting rid of the "Track distance:" prefix.

akinwale · 2024-02-09T02:22:40Z

@shubham1206agra Please also remember to test with Spanish.

On the BNP, the . should be a , when the user's language preference is Spanish.

melvin-bot · 2024-02-11T12:14:52Z

@akinwale Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

akinwale · 2024-02-11T20:41:54Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

34849-android-native.mp4

Android: mWeb Chrome

34849-android-chrome.mp4

iOS: Native

34849-ios-native.mp4

iOS: mWeb Safari

34849-ios-safari.mp4

MacOS: Chrome / Safari

34849-web.mp4

MacOS: Desktop

34849-desktop.mp4

akinwale · 2024-02-11T20:46:43Z

@shubham1206agra Please change the texts Track distance: Rate and Track distance: Unit to just Rate and Unit respectively. Everything else looks good.

shubham1206agra · 2024-02-12T05:05:59Z

@akinwale Done.

akinwale

LGTM.

src/components/AmountForm.tsx

src/components/Form/FormProvider.tsx

tgolen · 2024-02-12T16:30:48Z

src/components/Form/FormProvider.tsx

+        submitButtonStyles?: StyleProp<ViewStyle>;
+
+        /** Whether to apply flex to the submit button */
+        submitFlexEnabled?: boolean;


Why isn't this part of submitButtonStyles?

Tried this, but wasn't working correctly, so I decided to remove flex via prop.

Can you expand on that a little more? What wasn't working?

Could you make flex1 part of the default styles, and then in the one component where it currently has submitFlexEnabled=false, pass styles.flexReset?

flexReset with flex1 doesn't work for some reason. It applies flex1 only.

src/ONYXKEYS.ts

OSBotify · 2024-02-12T21:55:37Z

✋ 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 · 2024-02-15T05:23:31Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.41-12 🚀

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

hayata-suenaga · 2024-02-20T17:53:53Z

This PR causes the UI alignment issue

ikevin127 · 2024-06-15T20:21:20Z

src/libs/MoneyRequestUtils.ts

+function calculateAmountLength(amount: string, decimals: number): number {
    const leadingZeroes = amount.match(/^0+/);
    const leadingZeroesLength = leadingZeroes?.[0]?.length ?? 0;
-    const absAmount = parseFloat((Number(stripCommaFromAmount(amount)) * 100).toFixed(2)).toString();
+    const absAmount = parseFloat((Number(stripCommaFromAmount(amount)) * 10 ** decimals).toFixed(2)).toString();


This change caused issue #42084, some context:

In calculateAmountLength, we multiply the amount with 10 to the power of the number of decimals allowed on a currency. So, if the currency allows 2 decimals, then it will be multiplied by 100, that's why we can only input 8 digit length for currency like USD. But if the currency doesn't allow decimal, such as IQD, then we can input 10 digit length.

However, in the BE, all amounts are in cent, so the 10-digit length will be 12 digit length, and BE will return an error for it.

WIP: test form flow

c7be0e6

shubham1206agra added 7 commits January 20, 2024 18:35

WIP: minor visual fixes

aaa03ec

WIP: adding unit page, and amountForm component

7ae7b0a

WIP: bug fixes

126eedc

WIP: bug fixes 2

eafe71d

WIP: rate page implementation

ef54837

Visual fixes

bfa1572

prettier fix

e0060f6

Merge branch 'Expensify:main' into fix-workspace-rate-unit-flow

52d7672

akinwale suggested changes Jan 22, 2024

View reviewed changes

shubham1206agra commented Jan 22, 2024

View reviewed changes

src/pages/workspace/reimburse/WorkspaceRateAndUnitPage/UnitPage.tsx Outdated Show resolved Hide resolved

shubham1206agra commented Jan 22, 2024

View reviewed changes

src/pages/workspace/reimburse/WorkspaceRateAndUnitPage/UnitPage.tsx Outdated Show resolved Hide resolved

shubham1206agra commented Jan 22, 2024

View reviewed changes

src/pages/workspace/reimburse/WorkspaceRateAndUnitPage/RatePage.tsx Outdated Show resolved Hide resolved

shubham1206agra added 11 commits January 31, 2024 18:29

merge main

d9e62f8

merge main

0643d90

ts fix

6cee6d5

added better translations

cff1aee

corrected types

11c1529

using onyx instead of route query param to pass the values

c4cdb9b

Merge branch 'main' of https://github.com/Expensify/App into fix-work…

6683966

…space-rate-unit-flow

merge main

eb92a06

minor fixes

332f379

type fixes

efea853

prettier fix

c1bbbc2

shubham1206agra added 3 commits February 3, 2024 19:34

fixed error with changing value in AmountForm

c8cfca2

fix translations

ecc6de6

move onyx to actions

bbf64b1

Fix another navigation bug

f3e6326

shubham1206agra and others added 2 commits February 11, 2024 13:07

Merge branch 'main' into fix-workspace-rate-unit-flow

5cde139

Fix style layout of rate page

1d5a653

shubham1206agra marked this pull request as ready for review February 11, 2024 12:14

shubham1206agra requested a review from a team as a code owner February 11, 2024 12:14

melvin-bot bot requested review from akinwale and removed request for a team February 11, 2024 12:14

Fix header title

bae64bb

akinwale approved these changes Feb 12, 2024

View reviewed changes

melvin-bot bot requested a review from tgolen February 12, 2024 05:14

tgolen requested changes Feb 12, 2024

View reviewed changes

shubham1206agra added 2 commits February 12, 2024 22:59

Adding comments

16445e5

fix lint

6b91fe5

tgolen approved these changes Feb 12, 2024

View reviewed changes

tgolen merged commit 698b4e2 into Expensify:main Feb 12, 2024
15 checks passed

github-actions bot mentioned this pull request Feb 13, 2024

Deploy Checklist: New Expensify 2024-02-13 #36429

Closed

ZhenjaHorbach mentioned this pull request Feb 13, 2024

[HOLD for payment 2024-02-26] Reimbursements - Error message is not left aligned to other items #36436

Closed

6 tasks

marcochavezf mentioned this pull request Feb 13, 2024

Reimbursements - Focus (cursor) is not appearing in amount input field #36435

Closed

6 tasks

bernhardoj mentioned this pull request May 14, 2024

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] mWeb - IOU - Creating a split expense with IQD currency & 10 digit amount unexpected error shown #42084

Closed

6 tasks

ikevin127 reviewed Jun 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Workspace Rate Unit Form Flow #34849

Fix Workspace Rate Unit Form Flow #34849

shubham1206agra commented Jan 20, 2024 •

edited

Loading

melvin-bot bot commented Jan 20, 2024

shubham1206agra commented Jan 21, 2024

akinwale left a comment •

edited

Loading

akinwale Jan 22, 2024

shubham1206agra Jan 22, 2024

akinwale Jan 22, 2024

tgolen Jan 22, 2024

shubham1206agra Jan 23, 2024

tgolen Jan 23, 2024

shubham1206agra Jan 23, 2024

shubham1206agra Feb 3, 2024

shubham1206agra commented Feb 3, 2024

dubielzyk-expensify commented Feb 9, 2024

akinwale commented Feb 9, 2024

akinwale commented Feb 9, 2024

melvin-bot bot commented Feb 11, 2024

akinwale commented Feb 11, 2024 •

edited

Loading

akinwale commented Feb 11, 2024 •

edited

Loading

shubham1206agra commented Feb 12, 2024

akinwale left a comment

tgolen Feb 12, 2024

shubham1206agra Feb 12, 2024

tgolen Feb 12, 2024

shubham1206agra Feb 12, 2024

OSBotify commented Feb 12, 2024

OSBotify commented Feb 15, 2024

hayata-suenaga commented Feb 20, 2024

ikevin127 Jun 15, 2024

Fix Workspace Rate Unit Form Flow #34849

Fix Workspace Rate Unit Form Flow #34849

Conversation

shubham1206agra commented Jan 20, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 20, 2024

shubham1206agra commented Jan 21, 2024

akinwale left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1206agra commented Feb 3, 2024

dubielzyk-expensify commented Feb 9, 2024

akinwale commented Feb 9, 2024

akinwale commented Feb 9, 2024

melvin-bot bot commented Feb 11, 2024

akinwale commented Feb 11, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

akinwale commented Feb 11, 2024 • edited Loading

shubham1206agra commented Feb 12, 2024

akinwale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Feb 12, 2024

OSBotify commented Feb 15, 2024

hayata-suenaga commented Feb 20, 2024

Choose a reason for hiding this comment

shubham1206agra commented Jan 20, 2024 •

edited

Loading

akinwale left a comment •

edited

Loading

akinwale commented Feb 11, 2024 •

edited

Loading

akinwale commented Feb 11, 2024 •

edited

Loading