Update the chat UI to reflect drag-and-drop

[vladamx]

Details

1. Use state machine based approach to detect drag and drop over drop zone efficiently. Ignore events on the children. Currently, drag creates a lot of events for all children of drop zone therefore re-rendering ReportActionCompose very frequenly which made UI slugish. This PR fixes that as well.

2. Created UI as per Figma spec with translations.

3. Made sure there are no regressions in mobile environments.

Fixed Issues

$ https://github.com/Expensify/App/issues/11823

PROPOSAL: https://github.com/Expensify/App/issues/11823#issuecomment-1279864525

Tests / QA

1. Login to Expensify
2. Go to existing report or create new one
3. Find file you want to drag over to the Report
4. Drag over should be detected(drag overlay shown) in the messages and composer area.
5. Drop should not be detected if you leave the area by dragging over LHN, Report header or leaving the browser/desktop window
6. On drop in the drop zone area, it should trigger the modal with a file preview for supported file types.
7. On drop outside of drop zone it should ignore the drop. Should not open file in new window.

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

PR Review Checklist

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

https://user-images.githubusercontent.com/11381171/197220088-c530aade-f738-4892-be55-12d1277a5f9e.mp4

Mobile Web - Chrome

https://user-images.githubusercontent.com/11381171/197220274-6956ad15-d2a1-4bc9-937b-0b76417e8c43.mov

Mobile Web - Safari

https://user-images.githubusercontent.com/11381171/197220361-e3920cff-a6d1-4b4c-b448-cde1339cae10.mov

Desktop

https://user-images.githubusercontent.com/11381171/197220418-342a5138-5468-4404-9cf3-ba45e297443c.mov

iOS

https://user-images.githubusercontent.com/11381171/197220547-e94a209a-7d9c-4d79-a5a2-bba22f227ad4.mov

Android

https://user-images.githubusercontent.com/11381171/197220640-451dcc8c-dda6-4f9f-bac0-6c7648425d67.mov

[vladamx]

@shawnborton Updated

@parasharrajat Can you please take a look at this one? I see that you are not assigned as a reviewer.

[parasharrajat]

I think you removed the $ from. Please compare it with the latest PR template and fix it.

[parasharrajat]

Please use informative commit messages.

[vladamx]

Please use informative commit messages.

👍 This was just the base for the initial proposal

[vladamx]

@parasharrajat

Seems that logic for detecting dragleave on window does not work on firefox unfortunately.

After a little bit more of a research it seems that counter approach is the most reasonable thing that works cross browser.

[parasharrajat]

Ok, let's do that then.

[vladamx]

@parasharrajat I took a litle bit more time and got the idea to use clientBoundingRect to detect dragleave on dropzone and it works cross browser.

Ready for review.

[shawnborton]

Agree with @parasharrajat - just the background should have the slight transparency, not the icon and text.

[vladamx]

WEB: Firefox: I think there are still issues.

screen-2022-11-01_04.04.48.mp4

I am not able to reproduce this on Firefox 106.0.2 (MacOS). Which version are you using? Are you on the latest commit? Is it maybe that you have the browser zoomed in? Can you try on default zoom? Can anyone else reproduce this? @shawnborton @roryabraham

[parasharrajat]

Let me upgrade to the latest firefox versions and retest it on macOS. I will try to get the steps.

[vladamx]

Icon and text should not be opaque.

@parasharrajat Can you retest this as well? Not able to reproduce

[parasharrajat]

ISSUE: WEB: No Drop UI is shown on dragging an image.

Steps: 2. Resize the browser till the window Width is 730px. 3. Start dragging a file from the file manager over the chat body. 4. UI flashes and then no UI.

Browser: Chrome, firefox. OS: Linux

[parasharrajat]

Please merge main.

[vladamx]

ISSUE: WEB: No Drop UI is shown on dragging an image.

Steps: 2. Resize the browser till the window Width is 730px. 3. Start dragging a file from the file manager over the chat body. 4. UI flashes and then no UI.

Browser: Chrome, firefox. OS: Linux

@parasharrajat Please retest when you can. Thanks 👍

[vladamx]

I am curious. What is the advantage of adding this to the native platform as well? no drag and drop functionality is configured on native.

It can serve as an architecture setup for portal behavior in the future. Should not be too expensive.

[parasharrajat]

Ok. As currently there is no use of that, let's drop it and you can create noop index.native.js component for ReportDropUI

[vladamx]

Ok. As currently there is no use of that, let's drop it and you can create noop index.native.js component for ReportDropUI

By droping it, do you mean to make sure to not register PortalProvider and to not render PortalHost on native?

[parasharrajat]

• Remove the installed portal package.
• Use Portal from react-dom.
• You can create two files in reportDropUI index.js(web|Desktop) and index.native.js(native).
• Index.native.js will just create an empty component that returns null as we don't need this on native.

@roryabraham Does this seem fine to you? my reasoning is that we don't have dragNdrop on native so we don't need extra lib and configuration.

[vladamx]

@parasharrajat @roryabraham I think we wanted to avoid to depend on react-dom for portals at the first place and use cross platform solution everywhere.

Why reverting this now?

[roryabraham]

my reasoning is that we don't have dragNdrop on native so we don't need extra lib and configuration

@parasharrajat Switching to a cross-platform portal implementation is something I specifically asked @vladamx to do here (and you gave it a 👍🏼 😆).

It might be a bit of a pre-optimization, but ultimately I think it's okay because it's actually simpler in that it reduces the amount of platform-specific code we have to maintain. For a bit of additional context, I think it's totally possible we might see use-cases for Portals on the native platforms in the coming years. For example, an iPad user might install the iOS native app, and use it in wide-screen mode with a keyboard and touchpad. Or maybe they are using an Apple Pencil which provides hover support, and we want to show them tooltips (which use Portals as well I believe)

[parasharrajat]

Yeah, :smile: I gave it :+1: at that time for the same reason to have cross-platform portals. But now I think if it doesn't have a use-case, we should not add a library.

IMO, If we need this in the future, we can add it back and refactor the code but I don't see any such feature in 6 months timeline.

---

Also, keeping it does not create any issues. So it is fine too.

[vladamx]

Any ideas on how we might extract some of the DragNDrop code out of Composer to make it more generic/reusable?

We can probably compose. Will make a quick draft.

[vladamx]

Yeah, :smile: I gave it :+1: at that time for the same reason to have cross-platform portals. But now I think if it doesn't have a use-case, we should not add a library.

IMO, If we need this in the future, we can add it back and refactor the code but I don't see any such feature in 6 months timeline.

---

Also, keeping it does not create any issues. So it is fine too.

I think the lib is not noticeable overhead and it will serve in the future as a guideline on how to do portals.

[vladamx]

@roryabraham @parasharrajat Updated, please recheck

[parasharrajat]

Checking...

[parasharrajat]

BUG: WEB: Drop UI gets stuck when use cancels file drop. Steps:

1. Open the app on web browser.
2. Minimize the window to mobile dimensions(not required but helps to easily test).
3. Open the file manager.
4. Start dragging a file over the app.
5. Notice the drop UI is visible.
6. Drop the file out of the browser window cancelling the drop action.
7. Notice that drop UI does not hide(even on resize of window).

https://user-images.githubusercontent.com/24370807/200422791-eee39c53-d357-4938-b090-10696f37286c.mp4

[vladamx]

BUG: WEB: Drop UI gets stuck when use cancels file drop. Steps:

1. Open the app on web browser.
2. Minimize the window to mobile dimensions(not required but helps to easily test).
3. Open the file manager.
4. Start dragging a file over the app.
5. Notice the drop UI is visible.
6. Drop the file out of the browser window cancelling the drop action.
7. Notice that drop UI does not hide(even on resize of window).

screen-2022-11-08_03.17.18.mp4

@parasharrajat Added more checks, can you retest?

Sorry for the incovenience and thanks for testing - It seems there are quite a few edge cases to properly detect drag & drop cross browser

[parasharrajat]

Same issue on Safari mac. Could you please test it on all common browsers and attach videos for those to the PR?

https://user-images.githubusercontent.com/24370807/200699032-a31fdeca-afa4-4da2-9751-0db15402a490.mp4

[parasharrajat]

ok, now working fine on Firefox Linux. But there is one more phenomenon. I guess it should not happen if we are showing the full-screen drop UI.

BUG: web: the chat list scrolls when a user drags the image between the report header and the body.

https://user-images.githubusercontent.com/24370807/200700849-73603f64-e1de-4b7f-98f9-ec5da7c056c8.mp4

[vladamx]

ok, now working fine on Firefox Linux. But there is one more phenomenon. I guess it should not happen if we are showing the full-screen drop UI.

BUG: web: the chat list scrolls when a user drags the image between the report header and the body.

screen-2022-11-09_05.17.06.mp4

Researched a little bit but it seems there is no sane workaround for this behavior