[Refactor] WorkspaceNewRoomPage Form + Fix From nested children from class component

[fedirjh]

Details

• Refactor the Workspace New Room Page form to use Form.js.
• Fix Form.js to detect nested children inside custom class component.

Fixed Issues

#13524
PROPOSAL: #13524 (comment)

Tests / QA Steps

Test 1

1. Choose New Room from the create menu
2. Fill up the form
3. Click Create Room
4. Room Created successfully

Test 2

1. Choose New Room from the create menu
2. Skip the Room Name Input
3. Select Workspace
4. An Error Appears : Please enter a room name
5. Click on fix the errors
6. Room Name Input is focused
7. Enter this value : Test Room (Any value with Uppercase and space)
8. Room Name Input's value changed to lowercase and the space is replaced with underscore _

• UI looks as it did before the refactor
• Values can be added and edited
• Errors are highlighted correctly (input border)
• Error messages show up correctly
• Draft values are saved properly
• Form alerts are displayed correctly
• Clicking the fix the errors link focuses the first input with error
• No duplicate submission of the form occurs (when it's already submitting)
• Verify that no errors appear in the JS console

Offline tests

1. Choose New Room from the create menu
2. Fill up the form
3. Click Create Room
4. Verified that the form can be submitted and the button is enabled

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 the expected offline behavior in the Offline steps 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • 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 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 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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(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.
• 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

WorkspaceNewRoomPage Page - Refactored

WorkspaceNewRoomPage.mov

ReportSettingsPage - Not Refactored

Screen.Recording.2022-12-19.at.10.29.15.PM.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2022-12-19.at.10.55.40.PM.mov

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2022-12-19.at.23.19.58.mp4

Android

Android.mov

[melvin-bot]

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

[melvin-bot]

@Gonals 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]

[fedirjh]

@Gonals Looks like this pr should be assigned to @puneetlath and @mollfpr

[mollfpr]

@fedirjh We should add the test description. For the example here #12766

[fedirjh]

@fedirjh We should add the test description. For the example here #12766

@mollfpr thank you , tests description added

[mollfpr]

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 / Chrome
  • iOS / native
  • iOS / 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 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 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 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(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.
• 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

13713.Web.mov

Mobile Web - Chrome

13713.mWeb-Android.mov

Mobile Web - Safari

13713.mWeb-iOS.mov

Desktop

13713.Desktop.mov

iOS

13713.iOS.mov

Android

13713.Android.mov

[mollfpr]

Test online and offline with the HT account working well! Just need to resolve a comment #13713 (comment)

[mollfpr]

LGTM and test well 👍

All your's @puneetlath

[puneetlath]

Just a couple of small comments. Mostly looks good!

[melvin-bot]

Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	846.247 ms → 855.360 ms (+9.113 ms, +1.1%)
runJsBundle	205.250 ms → 208.563 ms (+3.313 ms, +1.6%)
regularAppStart	0.015 ms → 0.015 ms (+0.000 ms, +2.2%)
nativeLaunch	9.906 ms → 9.710 ms (-0.197 ms, -2.0%)

Show details

Name	Duration
TTI	Baseline Mean: 846.247 ms Stdev: 31.625 ms (3.7%) Runs: 795.9201120000798 800.6096700001508 800.8393699999433 802.8204510000069 804.8505259999074 805.4909920000937 815.811771000037 820.4313739999197 823.7367130001076 825.6293279998936 836.295158999972 838.5719830000307 840.7248249999247 841.4400929999538 843.3225360000506 847.1830319999717 849.467699999921 854.2150850000326 855.0396790001541 856.2652759999037 856.8625380001031 862.5922360001132 865.1683990000747 865.4737499998882 871.5500890000258 881.2852610000409 881.624928999925 885.6375939999707 886.7220330000855 889.419058999978 928.6450730001088 Current Mean: 855.360 ms Stdev: 41.761 ms (4.9%) Runs: 779.7502230000682 785.9799529998563 798.7377679999918 799.1039209999144 808.8868499998935 811.9893310000189 815.7627730001695 819.4341270001605 821.138398999814 821.7319280002266 831.5045759999193 838.220426000189 843.5274939998053 851.7752990000881 851.8083250001073 854.3166800001636 857.5800379998982 858.0375279998407 862.7424139999785 883.7125510000624 884.5006729997694 889.262506000232 889.6635590000078 892.985175000038 893.3025520001538 900.0609220000915 900.385135000106 906.4884279998951 912.7242890000343 914.1901110000908 936.8416109997779
runJsBundle	Baseline Mean: 205.250 ms Stdev: 28.676 ms (14.0%) Runs: 157 159 175 176 178 180 181 183 183 184 185 186 188 196 198 202 206 210 210 211 211 212 221 222 224 228 230 236 238 255 271 272 Current Mean: 208.563 ms Stdev: 24.032 ms (11.5%) Runs: 170 172 176 181 182 185 185 188 191 192 193 194 195 198 206 208 211 211 214 214 218 220 220 221 225 226 233 235 246 250 256 258
regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (5.5%) Runs: 0.013264999957755208 0.013427000027149916 0.013428000034764409 0.01346899988129735 0.01355000026524067 0.013672000030055642 0.013794000027701259 0.0138349998742342 0.013916000258177519 0.01411899970844388 0.014200999867171049 0.014240999706089497 0.014281999785453081 0.014362999936565757 0.01436399994418025 0.01436399994418025 0.014485999941825867 0.014527000021189451 0.014567000092938542 0.0147299999371171 0.015055000083521008 0.01525900000706315 0.015300000086426735 0.015461999922990799 0.015543999848887324 0.015583999920636415 0.01558400015346706 0.015625 0.015666000079363585 0.016032000072300434 Current Mean: 0.015 ms Stdev: 0.001 ms (7.2%) Runs: 0.012492000125348568 0.013428000267595053 0.01346899988129735 0.013590000104159117 0.013631000183522701 0.014037999790161848 0.014038000255823135 0.014200999867171049 0.014241000171750784 0.014283000025898218 0.014323000330477953 0.014403999783098698 0.01444500032812357 0.01448499970138073 0.014525999780744314 0.014606999699026346 0.014689000323414803 0.0147299999371171 0.014973999932408333 0.014973999932408333 0.015137000009417534 0.01525900000706315 0.01525900000706315 0.015339999925345182 0.015422000084072351 0.015585000161081553 0.01623600022867322 0.016276000067591667 0.016642000060528517 0.016642999835312366 0.01680499967187643 0.017009000293910503
nativeLaunch	Baseline Mean: 9.906 ms Stdev: 1.377 ms (13.9%) Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 11 11 11 11 11 11 11 11 11 12 12 12 13 Current Mean: 9.710 ms Stdev: 1.987 ms (20.5%) Runs: 7 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 10 10 10 11 11 11 12 12 12 12 14 16

[OSBotify]

🚀 Deployed to staging by @puneetlath in version: 1.2.43-0 🚀

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

[OSBotify]

🚀 Deployed to production by @chiragsalian in version: 1.2.43-1 🚀

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

[situchan]

Is there any reason to remove auto focus here?

[puneetlath]

I feel like we had a convo about this, but I don't recall why it was done. @fedirjh do you?

[fedirjh]

Is there any reason to remove auto focus here?

That workaround doesn’t work with the Form component , the Form override input’s ref here , then every passed ref will be undefined.

ref: node => this.inputRefs[inputID] = node,