Update the chat UI to reflect drag-and-drop #12056
Conversation
…3-drag-and-drop-overlay
src/styles/themes/default.js
Outdated
| @@ -48,4 +48,5 @@ export default { | |||
| placeholderText: colors.gray3, | |||
| heroCard: colors.blue, | |||
| uploadPreviewActivityIndicator: colors.gray1, | |||
| overlayBackgroundColor: colors.translucentWhite, | |||
There was a problem hiding this comment.
Similar to how we are a bit more specific for modalBackground - perhaps we do something similar and call this dragAndDropBackground
There was a problem hiding this comment.
I just realized I contradicted this in a later review. @shawnborton maybe you can provide some insight on why you prefer the more specific theme color here?
There was a problem hiding this comment.
overlayBackgroundColor just feels a bit too generic. Is that the overlay we use for modals? It's actually not, so maybe we should call it something more specific.
src/styles/colors.js
Outdated
| @@ -18,4 +18,5 @@ export default { | |||
| redDisabled: '#fea29a', | |||
| yellow: '#fed607', | |||
| transparent: 'transparent', | |||
| translucentWhite: 'rgba(255,255,255, 0.96)', | |||
There was a problem hiding this comment.
Rather than needing to add a new color value here, can we do something similar to what we've done for the modal background? Basically we just use the standard white color, but then add opacity as a separate style?
There was a problem hiding this comment.
Actually just realized I contradicted this review comment in a later review too. Sorry about the confusion
|
@shawnborton Updated @parasharrajat Can you please take a look at this one? I see that you are not assigned as a reviewer. |
|
I think you removed the $ from. Please compare it with the latest PR template and fix it. |
src/components/Composer/index.js
Outdated
| @@ -132,6 +133,15 @@ class Composer extends React.Component { | |||
| }; | |||
| this.dragNDropListener = this.dragNDropListener.bind(this); | |||
| this.paste = this.paste.bind(this); | |||
| if (document) { | |||
| this.dropZone = document.getElementById(CONST.REPORT.DROP_NATIVE_ID); | |||
| if (this.dropZone) { | |||
There was a problem hiding this comment.
Is there a possibility where dropZone will not be present? If no, please remove this check.
src/components/Composer/index.js
Outdated
| if (this.dropZone) { | ||
| this.dropZoneDragHandler = this.dropZoneDragHandler.bind(this); | ||
| this.dropZoneDragListener = this.dropZoneDragListener.bind(this); | ||
| this.dropZoneDragState = 'dragleave'; |
There was a problem hiding this comment.
Please add a comment. What is this state?
src/components/Composer/index.js
Outdated
| this.props.onDragOver(e, isOriginComposer); | ||
| break; | ||
| case 'dragenter': | ||
| dropZoneDragHandler = (e) => { |
There was a problem hiding this comment.
No arrow function syntax.
src/components/Composer/index.js
Outdated
| @@ -197,46 +207,73 @@ class Composer extends React.Component { | |||
| } | |||
|
|
|||
| /** | |||
| * Handles all types of drag-N-drop events on the composer | |||
| * DragEvent | |||
|
Please use informative commit messages. |
👍 This was just the base for the initial proposal |
|
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. |
|
Ok, let's do that then. |
|
@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. |
…3-drag-and-drop-overlay
src/styles/styles.js
Outdated
| right: 0, | ||
| bottom: 0, | ||
| backgroundColor: themeColors.dragAndDropBackground, | ||
| opacity: 0.96, |
There was a problem hiding this comment.
IMO this opacity should be defined in the theme file
…3-drag-and-drop-overlay
src/components/DragAndDrop/index.js
Outdated
| onDragOver: DragAndDropPropTypes.onDragOver, | ||
|
|
||
| shouldAcceptDrop: PropTypes.func, |
There was a problem hiding this comment.
Please add comments for these propTypes.
There was a problem hiding this comment.
Can we do this please so that I can approve this PR?
|
Thanks, will wrap this up in few minutes. |
Screenshots🔲 iOS / native
🔲 iOS / Safari
🔲 MacOS / Desktopscreen-2022-11-23_21.59.42.mp4🔲 MacOS / Chromescreen-2022-11-23_21.53.36.mp4screen-2022-12-03_03.57.28.mp4🔲 Android / Chrome
🔲 Android / native
|
|
Sorry for catching this so late, but can we use a new icon to match the new branding? You can use this one (feel free to rename it so that we overwrite the old one): document-add.svg.zip |
|
Good call @shawnborton @vladamx Could you please quickly do that? |
There was a problem hiding this comment.
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 Issuessection 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
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - 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 verified the steps for local testing are in the
- 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.
toggleReportand notonIconClick). - 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 Copylabel 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
- 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.
- 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 usingAvatarhave 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
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - 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
Avataris modified, I verified thatAvataris 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.
cc: @roryabraham
🎀 👀 🎀 C+ reviewed
|
Here is a better icon, sorry about that! drag-and-drop.svg.zip |
|
Can we just get an updated screen shot/video with the new icon? |
|
I think we are out of time budget for this PR? cc @roryabraham Can we allocate compensation for redesign changes and regression tests? |
|
@vladamx Could you update the recordings on the PR with the latest? @roryabraham This is ready for your review. |
|
@parasharrajat Updated |
|
cc: @roryabraham |
| const defaultProps = { | ||
| onDragOver: () => {}, | ||
| shouldAcceptDrop: (e) => { | ||
| if (e.dataTransfer.types) { |
There was a problem hiding this comment.
This block could use a comment, imo it's not super what logic we're implementing here. I think we're trying to ensure that Drag & Drop only appears for files and not other things (like links?)
| @@ -97,6 +99,8 @@ const oldTheme = { | |||
| placeholderText: colors.gray3, | |||
| heroCard: colors.blue, | |||
| uploadPreviewActivityIndicator: colors.gray1, | |||
| dropUIBG: 'rgba(6,27,9,0.92)', | |||
There was a problem hiding this comment.
I don't think this is correct, but we're not using the old theme for now anyways, so NAB.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
cc @francoisl since you are NewDot deployer atm. Wanted to mention that I spotted what appears to be a pretty bad bug/crash while testing https://expensify.slack.com/archives/C01GTK53T8Q/p1670528976295469 Unfortunately, I can't provide repro steps |
|
🚀 Deployed to production by @chiragsalian in version: 1.2.38-6 🚀
|
Details
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.
Created UI as per Figma spec with translations.
Made sure there are no regressions in mobile environments.
Fixed Issues
$ #11823
PROPOSAL: #11823 (comment)
Tests / QA
PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
Web-DragNDrop.mp4
Mobile Web - Chrome
Android-Chrome-DragNDrop.mov
Mobile Web - Safari
Safari-DragNDrop.mov
Desktop
Desktop-DragNDrop.mov
iOS
IOS-DragNDrop.mov
Android
Android-DragNDrop.mov