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-08-11] [$500] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari #9485

Closed
kavimuru opened this issue Jun 17, 2022 · 21 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

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:

  1. Access staging.new.expensify.com
  2. Sign into a valid account
  3. Open any chat
  4. Drag & Drop an image within the chat

Expected Result:

The user expects to see the "+" icon so that they know that this image can be uploaded this way

Actual Result:

On Safari, The "+" icon does not show unless the user quickly moves the cursor
Note: The "+" icon appears consistently on Chrome browser and on Desktop app

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web Mac Safari
  • Desktop App

Version Number:

Reproducible in staging?:

Reproducible in production?:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
Both videos of Safari and chrome added

Bug5613718_Plus_icon_on_MAC_Chrome.mp4
Bug5613718_Drag_and_Drop_on_Safari.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause internal team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jun 17, 2022

Triggered auto assignment to @srikarparsi (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@srikarparsi
Copy link
Contributor

Did a little investigating, bug is for all files, not just images on safari

@melvin-bot melvin-bot bot removed the Overdue label Jun 28, 2022
@melvin-bot melvin-bot bot added the Overdue label Jul 7, 2022
@srikarparsi srikarparsi added Monthly KSv2 and removed Weekly KSv2 labels Jul 13, 2022
@melvin-bot melvin-bot bot removed the Overdue label Jul 13, 2022
@srikarparsi srikarparsi added the External Added to denote the issue can be worked on by a contributor label Jul 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Jul 13, 2022
@srikarparsi srikarparsi removed their assignment Jul 13, 2022
@puneetlath
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

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

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

melvin-bot bot commented Jul 13, 2022

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari [$250] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari Jul 13, 2022
@puneetlath
Copy link
Contributor

Still waiting for proposals. Upping to $500.

@puneetlath puneetlath changed the title [$250] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari [$500] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari Jul 19, 2022
@andrewlor
Copy link
Contributor

andrewlor commented Jul 19, 2022

Proposal

Issue

For some reason calling e.preventDefault() when handling the dragover event unsets the cursor style in safari. I confirmed this by commenting out e.preventDefault(), and the cursor style was working as expected.

Solution

e.preventDefault() is there for another reason, so we don't want to remove that. But it seems like setting the dropEffect on each dragover event works. This event fires continuously as the cursor is dragging over the document. Currently the dropEffect is just set once on dragenter and the cursor enters the document.

dragNDropListener(e) {
let isOriginComposer = false;
const handler = () => {
switch (e.type) {
case 'dragover':
e.preventDefault();
this.props.onDragOver(e, isOriginComposer);
break;
case 'dragenter':
e.dataTransfer.dropEffect = 'copy';
this.props.onDragEnter(e, isOriginComposer);
break;
case 'dragleave':
this.props.onDragLeave(e, isOriginComposer);
break;
case 'drop':
this.props.onDrop(e, isOriginComposer);
break;
default: break;
}
};

case 'dragover':
    e.preventDefault();
    e.dataTransfer.dropEffect = 'copy';  // add this line
    this.props.onDragOver(e, isOriginComposer);
    break;

Result

demo.mov

@parasharrajat
Copy link
Member

parasharrajat commented Jul 20, 2022

@andrewlor Idea looks good to me. Could you please show us how will you do that?

We have an automated and standardized way how to handle the proposals, which helps us to stay efficient and on top of things. I would recommend checking out some we have an automated and standardized way how to handle the proposals, which helps us to stay efficient and on top of things. I would recommend checking out some previous jobs, which already got a contributor assigned to see how they posted the proposals. There is also this part in our contributor guidelines which you might find useful.

Please, also share the exact changes in code you had to make to fix this issue. You can see from all the other issues and contributors that we won't just take the code and run away, no need to worry about that :)

Thank you!

@andrewlor
Copy link
Contributor

@parasharrajat I updated my proposal with more details, and actually found a slightly better solution. Please let me know if there is anything else you need, otherwise I'll wait to move forward with the PR.

@parasharrajat
Copy link
Member

otherwise I'll wait to move forward with the PR.

Go through the Contributing guidelines https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md to understand the process.

@andrewlor
Copy link
Contributor

andrewlor commented Jul 20, 2022

Pause at this step until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet).
If your solution proposal is accepted by the Expensify engineer assigned to the issue, Expensify will hire you on Upwork and assign the GitHub issue to you.

At this point I am waiting for the proposal to be accepted? As I understand it I'm waiting for the github issue to be assigned to me and to be hired on upwork before creating the PR. Not sure what I'm missing.

@parasharrajat
Copy link
Member

parasharrajat commented Jul 20, 2022

Not sure what I'm missing.

Nothing, you are on track. 👍

Looks like your proposal is what I was expecting. @andrewlor's proposal looks good to me.

@andrewlor Please test your solution on all possible platforms including different browsers(Firefox, chrome, safari).

cc: @puneetlath

🎀 👀 🎀 C+ reviewed

@andrewlor
Copy link
Contributor

Testing on all platforms. I could check on ios/android/mweb to make sure there isn't any regression while attaching files, but I'm pretty sure this should not be applicable as it should not be possible to drag and drop.

chrome.mov
desktop.mov
firefox.mov
safari.mov

@parasharrajat
Copy link
Member

cc: @puneetlath #9485 (comment)

@puneetlath
Copy link
Contributor

Sounds simple and straightforward to me.

@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 2022

📣 @andrewlor You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 26, 2022
@melvin-bot melvin-bot bot added Overdue Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 4, 2022
@melvin-bot melvin-bot bot changed the title [$500] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari [HOLD for payment 2022-08-11] [$500] Chat - Safari - Drag & Drop - Little "+ copy" icon doesn't appear consistently on Safari Aug 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.87-9 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-08-11. 🎊

@melvin-bot melvin-bot bot removed the Overdue label Aug 4, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@puneetlath
Copy link
Contributor

Paid @andrewlor. Sent C+ contract to @parasharrajat.

@puneetlath
Copy link
Contributor

Everyone paid. Thanks for your contributions everyone!

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 Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants