Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2022-12-20] [$1000] [Feature] Update the chat UI to reflect drag-and-drop update ability #11823

Closed
roryabraham opened this issue Oct 13, 2022 · 70 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Design External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review

Comments

@roryabraham
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Drag an image file over a chat

Expected Result:

  1. The cursor should change to be a green +
  2. The UI of the chat should update to reflect that you can drop the file in there and it will be uploaded

Actual Result:

  1. The cursor changes to be a green +
  2. The UI of the chat does not update to reflect that you can drop the file in there and it will be uploaded ❌

Workaround:

n/a – this is polish to make an existing feature more intuitive.

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

There is no concept of drag-and-drop on iOS, Android, mWeb, so this is a feature request for web and desktop only.

Version Number: 1.2.14-0
Notes/Photos/Videos:

This is how the feature looks in slack:

image

Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665684156253079

View all open jobs on GitHub

@roryabraham roryabraham added Daily KSv2 NewFeature Something to build that is a new item. Design labels Oct 13, 2022
@roryabraham roryabraham self-assigned this Oct 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2022

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

@roryabraham
Copy link
Contributor Author

Bringing in @shawnborton for the mockups, then we can make this external.

@roryabraham roryabraham added Weekly KSv2 and removed Daily KSv2 labels Oct 13, 2022
@JmillsExpensify
Copy link

I think this was mentioned in the Slack thread, but we should think about what happens if someone tried to drag and drop over the LHN. Do we block that entirely? Do we highlight the individual row that is hovered over? I agree with @roryabraham that the chat window itself should be very straightforward, especially since we don't have threads and a right-docked side modal for reports yet.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 14, 2022

User example of this behaviour - user is trying to drag & drop a doc for bank verification and it's creating an expense instead. https://github.com/Expensify/Expensify/issues/233722

Should I file this GH as a bug? I closed it in favour of this GH but i can reopen if it's better.

@shawnborton
Copy link
Contributor

@JmillsExpensify for now I would suggest we just block the drag and drop action over the LHN, and only show feedback if the main chat pane is hovered. Will share screens shortly.

@shawnborton
Copy link
Contributor

Added a quick mock to the Slack thread, but here it is too:

image

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Oct 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2022

Triggered auto assignment to @maddylewis (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2022

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Update the chat UI to reflect drag-and-drop update ability [$250] Update the chat UI to reflect drag-and-drop update ability Oct 14, 2022
@roryabraham
Copy link
Contributor Author

Cool, design LGTM let's get an upwork job created for this, then we can start accepting proposals.

@parasharrajat
Copy link
Member

Calling all proposals...lol

@roryabraham
Copy link
Contributor Author

for now I would suggest we just block the drag and drop action over the LHN, and only show feedback if the main chat pane is hovered.

Quick clarification – I'm going to ask that we display the no-drop cursor style if you drag a file into the app anywhere where it's not allowed, which for now should include the whole LHN

@parasharrajat
Copy link
Member

And the report header as well. This is the default behavior of the browser. We are only listening for drop over report body and composer

@vladamx
Copy link
Contributor

vladamx commented Oct 16, 2022

Proposal

The idea is to detect if dragging over dropzone is active and to use React Portal to render the overlay in the drop view.

You can check the code in the diff below.
https://github.com/Expensify/App/compare/main...vladamx:App:vladamx-proposal-11823-drag-and-drop-overlay?expand=1

@parasharrajat
This should get you a good idea on the strategy.

Details like adding the document icon from design, extracting the overlay to separate file, making sure there is no regressions on mobile etc ; will be a part of PR

Outcome:

Chrome/ Web

Chrome.mp4

Desktop

Upload.-.Desktop.mp4

@maddylewis
Copy link
Contributor

created job posting here - https://www.upwork.com/jobs/~0198538cbc40aff5b9

@maddylewis maddylewis removed the Daily KSv2 label Oct 16, 2022
@roryabraham
Copy link
Contributor Author

roryabraham commented Dec 7, 2022

PR merged. I think the correct payout for this is:

  • $1000 base
  • Additional $250 for storybook stories: Update the chat UI to reflect drag-and-drop #12056 (comment)
  • Additional $500 that I'd like to authorize because the PR had a fair amount of scope creep, took a long time, and was working on shifting tides of the changing theme. @vladamx was flexible and persistent throughout the process, and I think that should be rewarded.

So $1750 total

@vladamx
Copy link
Contributor

vladamx commented Dec 7, 2022

@roryabraham That's fair, thank you!

@maddylewis
Copy link
Contributor

just noting here that i've seen this comment (#11823 (comment)) and will get this paid out before EOW.

@maddylewis
Copy link
Contributor

@roryabraham - will you confirm if we're paying out $1750 to both the C and C+ on this one?

@parasharrajat
Copy link
Member

@vladamx There is an issue from this PR https://expensify.slack.com/archives/C01GTK53T8Q/p1670528976295469. Could you please take a look?

@francoisl
Copy link
Contributor

In addition, I wanted to confirm - what's the expected behavior if you try to drag and drop a file while the right-hand panel is open?

The QA team opened this issue but it kinda makes sense to me not to allow drag-and-dropping a file in this case (or that would mean you'd want to drag and drop in the RHP?)

video_09.mp4

@parasharrajat
Copy link
Member

@francoisl I answered the same thing on the related issue.

@francoisl
Copy link
Contributor

Thanks, works for me and I agree with you here.

@parasharrajat
Copy link
Member

I think this error is happening because the child's componentDidMount is called before parent components. So it means that even though there is a microsecond difference in the mounting of these. Operations/calls are made in the wrong sequence. Thus the slightest delay will fix it.

In short, changing the execution order of dropzone mounting and document.getElementID(this.dropzone) will fix it which can be done as

        setTimeout(() => {
            this.dropZone = document.getElementById(this.props.dropZoneId);
            this.dropZoneRect = this.calculateDropZoneClientReact();
        });

But I am not suggesting this as a solution just the analysis.

@parasharrajat
Copy link
Member

parasharrajat commented Dec 9, 2022

It seems that #11823 (comment) is not related to this PR but is caused by some other PR. 😄

@maddylewis
Copy link
Contributor

im sorry for the delay on payment - will get this processed tomorrow.

@vladamx
Copy link
Contributor

vladamx commented Dec 12, 2022

@maddylewis i think we made a mistake with payment. I received 1750 + 1000 and i think i should receive only 1750 for this job

@maddylewis
Copy link
Contributor

uh oh! thanks for saying something @vladamx. is there a way in Upworks for you to reject the $1000 payment? i will also look into this on my end.

@maddylewis
Copy link
Contributor

@parasharrajat - will you confirm the payment you're expecting for this issue? thanks!

@parasharrajat
Copy link
Member

Same as #11823 (comment).


For an extra payment, the contractor can make a refund.

@vladamx
Copy link
Contributor

vladamx commented Dec 13, 2022

@maddylewis Hm, i don't seem to have that option. Maybe you can contact their support. Transactions are still in pending so they might be able to cancel $1000 transaction.

I can confirm that i received the payment for this issue.

@maddylewis
Copy link
Contributor

it sounds like i can request a refund. i will get on that. thanks again for letting me know, Vlad!

@maddylewis
Copy link
Contributor

hey @vladamx - i needed @michaelhaxhiu to request a refund in Upwork since i didnt have the permissions for some reason. we requested a $1k refund, but then received notification from Upwork that the refund request was $1750.

will you let me know the refund you were requested on your end? sorry for the hassle here!
image

@vladamx
Copy link
Contributor

vladamx commented Dec 13, 2022

@maddylewis i edited a refund to be 1000.
You should receive 900. 100 is service fee

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 13, 2022
@melvin-bot melvin-bot bot changed the title [$1000] [Feature] Update the chat UI to reflect drag-and-drop update ability [HOLD for payment 2022-12-20] [$1000] [Feature] Update the chat UI to reflect drag-and-drop update ability Dec 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.38-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2022
@mallenexpensify
Copy link
Contributor

Pretty sure this can be closed. Reopen and/or comment if not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Design External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants