Refactor companystep to use Form

[ravindra-encoresky]

Details

Refactor companyStep in to use Form.js component.

Fixed Issues

$ #9580
PROPOSAL: #9580 (comment)

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.

• Verify that no errors appear in the JS console

QA Steps

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• 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)
• I followed proper code patterns (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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and 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 followed the guidelines as stated in the Review Guidelines
• 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)
• 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 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(themeColors.componentBG)
• 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

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

• 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:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and 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 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(themeColors.componentBG)
• 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web.mov

Mobile Web - Chrome

mweb.MP4

Mobile Web - Safari

mobile_safari.MP4

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

[github-actions]

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.
✅ @ravindra-encoresky
❌ @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?

address-search-issue-validation-issue.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 : #9580 (comment)

I'm waiting.

[ravindra-encoresky]

@mananjadhav I have added back https://. Also updating you about last two pointers in some time.

[ravindra-encoresky]

@mananjadhav
2. We have taken incorporationDate as string in reimbursementAccountDraftPropTypes but on selecting date from date picker it return Date object. That's why it is giving error. It is an existing issue which is also in the old implementation. To address this issue we need to refactor reimbursementAccountDraftPropTypes which I think is out of scope of this PR

3. This is related to Form.js implementation. I have checked on other screens where CheckboxWithLabel is child of Form.js, same issue everywhere. So I think it is also out of scope of this PR.

[mananjadhav]

Thanks for the comments @ravindra-encoresky I'll test once again today with the default value for website and update the screencasts. Appreciate your patience on this one.

[luacmartins]

Just came back from ooo, I'll review this in a moment.

@mananjadhav ios is working for me. I'd recommend clearing node_modules and npm cache and running npm install again.

[mananjadhav]

Reviewer Checklist

I was able to get it working. Thanks for the patience here @ravindra-encoresky. Tests well.

I'd recommend clearing node_modules and npm cache and running npm install again.

This worked. Thanks @luacmartins. Been switching between multiple PRs today ;)

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:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and 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 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(themeColors.componentBG)
• 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web-company-step.mov

Mobile Web - Chrome

mweb-chrome-company-step.mov

Mobile Web - Safari

mweb-safari-company-step.mov

Desktop

desktop-company-step.mov

iOS

ios-company-step.mov

Android

android-company-step.mov

[luacmartins]

@ravindra-encoresky can you please merge main? It seems like we added defaultValues to AddressForm in this PR that was just merged - https://github.com/Expensify/App/pull/12766/files#diff-d3912c1b6eb5974ed910457e3932c818cd9931e055beaf7b3ca7cb6d1192fde6. There's no conflict because they were defined in different places, but this will result in us defining those twice!

[luacmartins]

In addition to merging main, I left a comment about reimbursementAccount and reimbursementAccountDraft props

[luacmartins]

@ravindra-encoresky I left a few comments above. We are really close, let's try to get this merged today!

[ravindra-encoresky]

@mananjadhav I've pushed the changes.
thanks. Looking forward.

[ravindra-encoresky]

@mananjadhav @luacmartins I had changed my system few days ago and due to this I have pushed one commit with the different account. I think some details is lost now, commits are showing unverified and CLA is not signed. Please guide me what step to take next.

P.S. All the code related changed are done. You can continue review

[sketchydroide]

@mananjadhav @luacmartins I had changed my system few days ago and due to this I have pushed one commit with the different account. I think some details is lost now, commits are showing unverified and CLA is not signed. Please guide me what step to take next.

@ravindra-encoresky You need to open a new PR and add all the commits you added to this one with the user that is approved. I think that should fix it.
I would advise to try one commit first and see if it works and once it does you can add all the other ones.

After that just add all the reviewers from this one to the next one.

[luacmartins]

@ravindra-encoresky you can try the following instructions:

1. git reset 0b554e73c73cd7acaba4eb881bf210c841072350 this will reset your branch to before the commit with your other account
2. Force push your changes
3. Merge main again and create a new commit with the changes from your latest commit
4. Run git -c core.editor=true rebase --exec 'git commit --amend --no-edit -n -S' -i $COMMIT_HASH replacing $COMMIT_HASH with the first commit that needs to be signed, in this case 6c9c88517df4a9dc6b26665be61d7a7f8167b44b
5. Force push your changes

If that fails, then @sketchydroide steps above sound fine to me.

[sketchydroide]

Thanks @luacmartins Your solution seems way better than mine.
@ravindra-encoresky let us know if that helped

[luacmartins]

Let’s see if it works 😬 🤞

[ravindra-encoresky]

@luacmartins Thanks for solution. I am on it. GPG key sign in is not working in my m2 macbook, I am currently resolving it. will update you once done.

[ravindra-encoresky]

@luacmartins @mananjadhav @sketchydroide I have opened a new PR #13076.

I have put all changes in just single commit because i was also getting issues in singing previous commits.

Please have a look.

[sketchydroide]

thanks @mananjadhav let's move there, we can close this PR