[HOLD] Refactor companystep to use Form

[ravindra-encoresky]

Details

Refactor companyStep in to use Form.js component.

Fixed Issues

$ #9580

Tests

1. Add a new workspace
2. Navigate to Settings > [Workspace] > Connect bank account > Connect manually.
3. Fill information on connect bank account page then navigate to company information page.
4. Fill company step form and validate changes.

• [x] Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] 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)
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] iOS / native
  • [x] Android / native
  • [x] iOS / Safari
  • [x] Android / Chrome
  • [x] MacOS / Chrome
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] 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)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product was added in all src/languages/* files
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] Any functional components have the displayName property
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] 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)
  • [x] 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)
  • [x] All JSX used for rendering exists in the render method
  • [x] 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
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] 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)
• [x] 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.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

• [x] I have verified the author checklist is complete (all boxes are checked off).
• [x] I verified the correct issue is linked in the ### Fixed Issues section above
• [x] I verified testing steps are clear and they cover the changes made in this PR
  • [x] I verified the steps for local testing are in the Tests section
  • [x] I verified the steps for Staging and/or Production testing are in the QA steps section
  • [x] 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)
  • [x] 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)
• [x] I checked that screenshots or videos are included for tests on all platforms
• [x] I verified tests pass on all platforms & I tested again on:
  • [x] iOS / native
  • [x] Android / native
  • [x] iOS / Safari
  • [x] Android / Chrome
  • [x] MacOS / Chrome
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I verified proper code patterns were followed (see Reviewing the code)
  • [x] 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).
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product was added in all src/languages/* files
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I verified that this PR follows the guidelines as stated in the Review Guidelines
• [x] 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)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] Any functional components have the displayName property
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] 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)
  • [x] 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)
  • [x] All JSX used for rendering exists in the render method
  • [x] 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
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] 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)
• [x] 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.
• [x] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

QA Steps

• [x] Verify that no errors appear in the JS console

Screenshots

Web

https://user-

images.githubusercontent.com/35763921/187890158-74dbde2a-e651-475f-8881-d3db1f43a17a.mov

Mobile Web

https://user-images.githubusercontent.com/35763921/188393613-18bf36e4-787c-4511-8936-d77ca1a397f9.MP4

Desktop

https://user-images.githubusercontent.com/35763921/187885205-de6fb00d-ecb5-45fe-8f8d-9b96bd394cec.mov

iOS

https://user-images.githubusercontent.com/35763921/187890279-ddd0bd70-f38e-4529-995c-29a797e1ce7b.mov

Android

https://user-images.githubusercontent.com/35763921/187890400-acb0237d-2b11-476e-ac05-c1e5de582c75.mov

[github-actions[bot]]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

1 out of 2 committers have signed the CLA.
:white_check_mark: @ravindra-encoresky
:x: @estbalveer
_{You can retrigger this bot by commenting recheck in this Pull Request}

[ravindra-encoresky]

I have read the CLA Document and I hereby sign the CLA

[mananjadhav]

I'll review and test this by tomorrow.

@ravindra-encoresky You need to mark every item in the checklist to confirm you've gone through it even if it doesn't apply to the PR. Also I hope you've gone through the guidelines mentioned in the issue GH body.

[ravindra-encoresky]

@mananjadhav checked all items in review checklist

[mananjadhav]

@ravindra-encoresky The changes look fine, but can you please take the latest main pull for this one?

Also mWeb screencast is missing. can you please update it? Once done ping me and I'll get to testing it.

[ravindra-encoresky]

hey @mananjadhav I have uploaded mWeb screen recording. Also merged main branch into this one.

[mananjadhav]

@ravindra-encoresky Changes look fine but I see one test failing. I am unable to select the correct address and it seems due to this validation on the city also fails. Attached is the screencast for the same. Can you please fix this? Also for all the platforms can you please add success (movement to the next step) as well as failure scenario?

https://user-images.githubusercontent.com/3069065/188514394-613966c2-6311-4738-a0cf-4f98c6333051.mov

[ravindra-encoresky]

okay, I am making necessary changes. will push once done.

[ravindra-encoresky]

@mananjadhav I have facing issue in selecting company address, It is a searchable input so on selecting suggested address it should fill up city, state, zip code also. But in current Form implementation I am unable to do so.

1. Generally we don't pass any value to inputs within Form component, but In this case I have to values to city, state and zip code so these value fill up after selecting address.
2. When I am passing values in defaultValue prop, it does not work.
3. When I am passing values in value prop, in this case value filling up is working but It failed form validation if we go and come back to page again.
4. Last option I have to exclude address inputs from Form component and manage their validation manually otherwise we have to make changes in our form component

[mananjadhav]

@ravindra-encoresky I've posted a comment here wrt to AddressForm component

[sketchydroide]

@ravindra-encoresky any updates? 😄

[ravindra-encoresky]

@ravindra-encoresky any updates? 😄

please check this comment of @mananjadhav here : https://github.com/Expensify/App/issues/9580#issuecomment-1241055886

I'm waiting.

[sketchydroide]

Sorry @ravindra-encoresky I look at the PRs more often than I look at the GHs, just updated the GH

[sketchydroide]

I'm adding the hold title to this PR as it's easier to know while looking at the list of PRs I need to review

[ravindra-encoresky]

@mananjadhav @sketchydroide I have used the updated AddressForm component and made the necessary changes. PR is ready for review.

[mananjadhav]

@ravindra-encoresky Thanks for the update on the PR Can you follow this template for the checklist and the screenshots?

We now have two test platforms for mWeb - Android Chrome and iOS Safari. Also your Web URL seems to be broken and hence the preview is not loaded.

Lastly we can remove the [HOLD] from the PR title.

[mananjadhav]

Did a quick review and changes seem to be looking good. @ravindra-encoresky Can you please resolve conflict? and address the earlier comment ?

[ravindra-encoresky]

@mananjadhav sure, I am working on it, I will let you know when done.

[ravindra-encoresky]

@mananjadhav I have made the necessary changes

1. Used new PR template.
2. Attached video for mobile safari
3. Merged main branch and resolved conflicts

[ravindra-encoresky]

@mananjadhav I have made the necessary changes

1. Used new PR template.
2. Attached video for mobile safari
3. Merged main branch and resolved conflicts

@mananjadhav

[luacmartins]

Also, I think that we can remove the hold on this PR.

[mananjadhav]

@ravindra-encoresky Did you get a chance to look at @luacmartins's comments?

[ravindra-encoresky]

@ravindra-encoresky Did you get a chance to look at @luacmartins's comments?

Will check.

[0xmiroslav]

https://github.com/Expensify/App/issues/12436 is closed in favor of this PR. So I think this needs to be tested here. I am sure that test will happen but just to confirm! cc: @mananjadhav

[ravindra-encoresky]

@luacmartins I have fixed above mentioned changes. Please check

[ravindra-encoresky]

@luacmartins I have fixed above mentioned changes. Please check

@mananjadhav @sketchydroide

[ravindra-encoresky]

Looking good. Just a few more comments:

1. Let's make reimbursmeentAccountDraft prop optional, since the user could be on the CompanyStep, login and not have any draft values saved. We can default it to an empty object.
2. Fix the errors doesn't seem to work on iOS:

fix-error.mov

@luacmartins

1. Made reimbursmeentAccountDraft optional as suggested.
2. I have checked it, on first click it works as expected but on second click input is already focused so it does nothing. On web on second click it scrolls up to make focused input visible but in iOS it's not.

If we consider it a bug then it require to modify Form.js

[sketchydroide]

I have checked it, on first click it works as expected but on second click input is already focused so it's do nothing. On web on second click it scroll up to make focused input visible but in iOS it's not.

I would say it should scroll up as well here for all the platforms where the input is not visible. but maybe this is a bug that goes beyond this PR if it envolves changing behaviours on Form.js, @luacmartins you know more about Forms than me, what do you think?

[luacmartins]

Sorry about the late reply - I’m ooo until Nov 22 so I’m checking these messages sporadically. I think it’s fine to fix the “fix the errors” behavior in a separate PR since that seems to be broken on other flows as well.

[JmillsExpensify]

Is this PR ready for another round of reviews? Or more generally, where are we at?