Implement Invite message page

[abdulrahuman5196]

@thienlnam @mollfpr

Details

Implementing a new Workspace members invite message page to fix the overlaying of message in the invite members page.

Fixed Issues

$ https://github.com/Expensify/App/issues/15083 PROPOSAL: https://github.com/Expensify/App/issues/15083#issuecomment-1441221893

Tests

1. Go Workspace -> Manage Members -> Invite Members
2. Choose members to invite -> Next
3. New invite message page to be shown with choosen members and invite message
4. Save should add the members to workspace

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

Offline tests

1. Go Workspace -> Manage Members -> Invite Members
2. Choose members to invite -> Next
3. New invite message page to be shown with choosen members and invite message
4. Save should add the members to workspace

QA Steps

1. Go Workspace -> Manage Members -> Invite Members
2. Choose members to invite -> Next
3. New invite message page to be shown with choosen members and invite message
4. Save should add the members to workspace

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

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 the expected offline behavior in the Offline steps 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 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).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android / native
  • [x] Android / Chrome
  • [x] iOS / native
  • [x] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [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 is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] 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.
  • [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] I verified that if a function's arguments changed that all usages have also been updated correctly
• [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] 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 any new file was added I verified that:
  • [x] 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
• [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] 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.
• [x] 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.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

https://user-images.githubusercontent.com/46707890/223535362-04f5a0d4-9551-4417-a290-22c0845794f5.mp4

Mobile Web - Chrome

https://user-images.githubusercontent.com/46707890/223535383-2bc0198e-5bc1-4989-8ffa-992241321968.mp4

Mobile Web - Safari

https://user-images.githubusercontent.com/46707890/223535415-10c0243e-4f17-49e3-9a95-9951be610df9.mp4

Desktop

https://user-images.githubusercontent.com/46707890/223535467-fd3ed23a-d1f6-458a-9963-727617d94144.mp4

iOS

https://user-images.githubusercontent.com/46707890/223535494-e0a973d3-b04b-4a51-964c-22079f17e552.mp4

Android

https://user-images.githubusercontent.com/46707890/223535526-558af778-0eb8-41bd-9a16-886ea0ef4580.mp4

[MelvinBot]

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

[MelvinBot]

@thienlnam @mollfpr One of you needs to 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]

[mollfpr]

@abdulrahuman5196 Could you resolve the conflict? Thanks!

[abdulrahuman5196]

@abdulrahuman5196 Could you resolve the conflict? Thanks!

Resolved the conflicts

@mollfpr

[mollfpr]

@abdulrahuman5196 We don't have the +1 avatar if more than 4 members have been added to the list.

https://user-images.githubusercontent.com/25520267/224711393-8d038634-6b56-4f6e-a42e-7573700a369b.mov

[mollfpr]

The avatar is also not showing on the iOS

https://user-images.githubusercontent.com/25520267/224719291-c92b3799-8335-4b86-b000-15ed783a0e24.mp4

[abdulrahuman5196]

@mollfpr Could you kindly check if you are using the latest commit from the PR. Its working fine for me. I have been merging with main to resolve conflicts.

[mollfpr]

@abdulrahuman5196 Thanks; I'll test it again with the latest commit. I might be missing something before.

[mollfpr]

@abdulrahuman5196 Could you update the PR to make the WorkspaceInvitePage and WorkspaceInviteMessagePage can work with Form.js under the hood? Also, check if the welcomeNote update when locale change is working too.

[abdulrahuman5196]

@abdulrahuman5196 Could you update the PR to make the WorkspaceInvitePage and WorkspaceInviteMessagePage can work with Form.js under the hood? Also, check if the welcomeNote update when locale change is working too.

@mollfpr The new page WorkspaceInviteMessagePage already uses Form component under the hood. But I didn't change the existing implementation of WorkspaceInvitePage to use Form component, do we need to update that as well to Form as part of this PR given that is an existing implementation?

And given this is a big PR, Can we do a full review of PR once(I am not sure if that was already done) and post the review comments alltogether, so that I can solve the comments in one shot. It will reduce the time for both of us, if not we might end up doing minor changes on every review.

[mollfpr]

@abdulrahuman5196 I Found this regression when searching for a new user outside the list. The new user avatar is not showing.

Step:

1. Open invite new members' workspace
2. Search for a user that is not on the list and select it
3. Press a next button

https://user-images.githubusercontent.com/25520267/225638428-f1aa8f60-b964-40c0-b357-1aab71fe88ef.mov

---

Yes, you use the Form.js in the invite message page, but the form state is still managed in that component (value and onChangeText props). I'll confirm it with someone if that is okay. I also expected we would have a draft value because we use Form.js for the invite message page.

[mollfpr]

@luacmartins Hey, I need your help here! We try using Form.js for the new page WorkspaceInviteMessagePage to send the message to the new member. However, in this PR, the input value is not handled by Form.js, so we should let Form.js take it. Here's the page I'm talking about.

Second, this PR is significant in solving the original issue. Could we use the original implementation and handle the refactor to use Form.js on other issues? cc @thienlnam

[abdulrahuman5196]

@luacmartins Hey, I need your help here! We try using Form.js for the new page WorkspaceInviteMessagePage to send the message to the new member. However, in this PR, the input value is not handled by Form.js, so we should let Form.js take it. Here's the page I'm talking about.

Second, this PR is significant in solving the original issue. Could we use the original implementation and handle the refactor to use Form.js on other issues? cc @thienlnam

@mollfpr I agree here on the callouts. I made the new implementation with the latest features available, but I intentionally made less refractor on existing code logic unrelated to the problem like 'default message and its translation logic' to avoid any unwanted regressions. But if we want to refractor existing logics like refractoring WorkspaceInvitePage to use Form.js and other refractors we can still do, but it might not add value to solution we are trying to solve.

So a confirm here would be good before I can start making those changes, if we still want to refractor existing code?

[abdulrahuman5196]

Summarizing open questions to reviewers to make everyone's life easier. Let me know if i had missed any. @mollfpr @luacmartins @thienlnam

1. https://github.com/Expensify/App/pull/15672#discussion_r1136899731 Concern: "I understand that the welcomeNote changes when the locale changes, but the Form will not be valid if we manage the value in the local component. We can set a new draft value for the form whenever the locale is changed, and we don't need value nor onChangeText in this input." Next action: This requires a fix in TextInput component itself to support our usecase as mentioned here - https://github.com/Expensify/App/pull/15672#discussion_r1139341904. We should create a separate issue fix to have this support in TextInput itself and do appropriate changes to the new page created as part of this PR.

2. https://github.com/Expensify/App/pull/15672#issuecomment-1472319358 Concern: "this PR is significant in solving the original issue. Could we use the original implementation and handle the refactor to use Form.js on other issues?" Response: This was intentionally made. But if we want to refractor existing logics like refractoring WorkspaceInvitePage(members selection page) to use Form.js and other refractors we can still do, but it might not add value to solution we are trying to solve. So we can create separate issue to do refractor as an improvement since this PR is already bloated. Next action item: We need confirmation from reviewers if we want to do that refractor or not as part of this PR.

@mollfpr For other review comments you have posted, I agree on them. As all other seems minor code changes, I will sort out all of them once we get the alignment on the above 2 open items.

[mollfpr]

Thank you @abdulrahuman5196, for the summary. I don't have any concerns with creating a new issue for the form refactor. @thienlnam @luacmartins I'll let you guys decide it.

[mollfpr]

@luacmartins @thienlnam Could you confirm 2 points of @abdulrahuman5196 summary here so we can move forward with the PR? Thank you!

[thienlnam]

I agree with the summary, this PR is already getting pretty large

[abdulrahuman5196]

@mollfpr @thienlnam @luacmartins I have addressed all the comments mentioned and pushed the required changes. Kindly review and let me know if more is required. Note: Kindly delete local checkout and make a new checkout to test my changes as I had to rebase this PR branch to remove the unsigned commits

[abdulrahuman5196]

@mollfpr Good catch on the regression and the order change in the workspace list. I have fixed the same and also updated the styles as per the recommendations. Kindly check and let me know if anything else is required from my end.

[MonilBhavsar]

Hi, Just noting that we had this bug on mWeb on old Invite members page https://github.com/Expensify/App/issues/10906. I would say we test this flow to make sure it works.

[abdulrahuman5196]

@mollfpr Any updates on the review? I think I have closed out on all existing comments. Kindly let me know if anything else needs to be done at my end

[mollfpr]

@abdulrahuman5196 Sorry for the delay! It seems looks good now, I’ll finish the review today!

[mollfpr]

Here are the regression I found @abdulrahuman5196

1. The last members draft should be clear after submit

https://user-images.githubusercontent.com/25520267/229288608-ffbabd6a-e964-47ea-a6ba-ec343f765da4.mov

2. I can submit an invite to the workspace with no selected user. Current staging is disabling the button when no user is selected. After the submission, there are errors user in the list?

https://user-images.githubusercontent.com/25520267/229288646-0264c9a1-fbc0-4ff5-b530-ec32254d5118.mov

3. Privacy link is not visible when the keyboard opens. Based on the design, the link is visible.

https://user-images.githubusercontent.com/25520267/229288777-66bf8263-cc93-4431-a19b-9e1974176eb0.mp4

[mollfpr]

@abdulrahuman5196 That's all for me. Everything else is seems working fine.

[abdulrahuman5196]

@mollfpr
for 1) I have fixed it. for 2) Currently there is no disable option in form.js submit component - https://github.com/Expensify/App/blob/main/src/components/Form.js#L326. I assume it has to do with us showing error message when user presses submit. But our usecase seems to be a special, so I can either expose disabled props from form.js and populate it via our page so that the button is disabled or show errors message via validate but the user won't be able to take any action by pressing fix the errors. So what should be the expected behaviour here for 3) I think we have to move the footer component into the FormAlertWrapper component from form.js(current implementation) https://github.com/Expensify/App/blob/main/src/components/FormAlertWrapper.js#L53. Wanted to confirm if the approach is ok as its a core component

[mollfpr]

so I can either expose disabled props from form.js and populate it via our page so that the button is disabled or show errors message via validate but the user won't be able to take any action by pressing fix the errors.

I'm unsure about this https://github.com/Expensify/App/blob/main/contributingGuides/FORMS.md#submit-button-disabling. But that is the straightforward way.

@luacmartins Is this okay to expose the disabled option for the submit button in Form.js?

---

I think we have to move the footer component into the FormAlertWrapper component from form.js(current implementation) https://github.com/Expensify/App/blob/main/src/components/FormAlertWrapper.js#L53. Wanted to confirm if the approach is ok as its a core component

I think that's okay. What do you think guys? @thienlnam @luacmartins

[thienlnam]

I think we have to move the footer component into the FormAlertWrapper component from form.js(current implementation) Expensify/App@main/src/components/FormAlertWrapper.js#L53. Wanted to confirm if the approach is ok as its a core component

I think that's okay. What do you think guys? @thienlnam @luacmartins

Probably a better question for @luacmartins since he added the form component. I think it's fine if the Privacy link is not visible when the keyboard opens but will let Carlos make the decision

[luacmartins]

@abdulrahuman5196

2. The way we should handle this is by showing an error on validate. Can we focus the search text input when the user presses fix the errors?

3. I think the Privacy link looks a bit odd below the Invite button. Has this design been approved by our design team?

[abdulrahuman5196]

@luacmartins

2. The way we should handle this is by showing an error on validate. Can we focus the search text input when the user presses fix the errors?

I am not sure if it would be meaningful. The user can't take any action even if we focus on the text input, any input doesn't make the submit available since there is no members selected. Should we show some specific errors in this case? Some design input would be great.

3. I think the Privacy link looks a bit odd below the Invite button. Has this design been approved by our design team?

yes. It was the initial design mocks provided - https://github.com/Expensify/App/issues/15083#issuecomment-1441221893

[mollfpr]

We can use the DotIndicatorMessage component to show the message.