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

Fix dragndrop upload #1867

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
2 participants
@eyal0
Copy link
Contributor

commented Apr 13, 2017

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch, or maintenance if it's
    a bug fix for an issue present in the current stable version (no PRs
    against master or anything else)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance or devel please),
    e.g. dev/my_new_feature
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase your PR if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated with
    lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

Feel free to delete all this help text, then describe
your PR further. You may use the template provided below to do that.
The more details the better!


What does this PR do and why is it necessary?

Fixes #202 so that the drag and drop upload doesn't disappear when the mouse stops moving.

How was it tested? How can it be tested by the reviewer?

I was able to reproduce the original issue and also see that this solution solves the issue. The reviewer should test that this doesn't break drag and drop on platforms that were already working.

Any background context you want to provide?

What are the relevant tickets if any?

Fixes #202

Screenshots (if appropriate)

Further notes

Another easier solution is to simply change the timeout from 100 to 400 as described in the issue. it's a smaller fix but a little clunky in that the page becomes less responsive.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2017

I might be overlooking something here, but with the changes I'm getting this behaviour in Chrome 57.0.2987.133 and Firefox 52.0.2 under Windows 10:

pr_1867

It appears that the drop is never detected properly and hence the overlay is not removed. It works fine when just moving the dragged file out of the window, but actually dropping it blocks the UI for good. I assume you didn't see that in your own tests and we have a case of different behaviour across operating systems here?

@eyal0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

Oy, I completely forgot to test that! I only tested dragging away from the screen and not actually uploading! Sorry.

I tested uploading, both to local and to a non-exsistant SD card. In both cases, the overlay goes away.

Thanks for testing!

@eyal0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

I fixed it, by the way. Try again?

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 19, 2017

Thanks, that looks better :) I'll squash this and cherry pick it to the maintenance branch - since it's a bug fix it makes sense to push it out as fast as possible and that will ensure it will be in 1.3.3. However that will mean that Github won't detect this PR as merged. I hope that's ok for you nevertheless.

foosel added a commit that referenced this pull request Apr 19, 2017

Fix dragndrop upload
See #1867. Fixes #202.

(cherry picked from commit d658c64)
@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 19, 2017

This is now merged on maintenance as 358a252, will be in the 1.3.3 release (and soon also merged up to devel). Thanks! 👍 Closing this manually since Github doesn't handle cherry-pick style merges sadly, don't be alarmed by that please.

@foosel foosel closed this Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.