Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Nov 26, 2016

Continuation of #90 (closed accidentally and force-pushed).

TODO:

  • special folders hack (e.g. favourites search folder)
  • visual drop feedback
  • add drag/drop scopes to prevent cross-account moves
  • message removal animation
  • subfolders are broken

fixes #24 💸 💰 💵 💃
fixes #201

@ChristophWurst
Copy link
Member Author

Alright. Basic move operations seem to work now. Also for nested folders. @Gomez @jancborchardt @nextcloud/mail check it out. Let me know if you experience any bugs. If you have no objections I'd like to add the visual feedback in a follow-up PR.

@ChristophWurst
Copy link
Member Author

I've made some small and not necessarily related changes:

  • removed more of the useless client-side cache.
  • don't trigger reload folders if we have fetched them already.
  • don't remove message pages. reload removes them however, because the server-side impl limits to 20 messages.

@ChristophWurst
Copy link
Member Author

@nextcloud/mail please review. I'd like to push a new release ASAP and this would be a nice feature for it :-)

@jancborchardt
Copy link
Member

Hey @Quix0r @plastikschnitzer @seal20 @pierreozoux @MeCias @djibux @GIT-STATA @pierreozoux please check if moving mails works for you with this pull request. :)

@plastikschnitzer
Copy link

I tried the latest nightly built version of the mail app. After uploading files to the server, the mail app is not loading any more, just a blank page.

@ChristophWurst
Copy link
Member Author

@plastikschnitzer weird. do you see anything related in the server log or browser log? I assume you've used NC10 right?

@irgendwie
Copy link
Member

irgendwie commented Nov 28, 2016

I am able to move mails between folders 👍 so the basic functionality works 🎉

Bugs:

  • Sometimes the mail icon freezes and stays at the current position (not sure when)
  • Drag the icon over to the mail body (the mail icon stays at the list), release the mouse button (on the mail body) and go back to the mail ist -> you now have a floating mail icon next to your cursor
  • (Maybe other small glitches)

@ChristophWurst
Copy link
Member Author

Sometimes the mail icon freezes and stays at the current position (not sure when)

Check your browser's log. This happened to me too but only when there was an error in the js code.

Drag the icon over to the mail body (the mail icon stays at the list), release the mouse button (on the mail body) and go back to the mail ist -> you now have a floating mail icon next to your cursor

So you've dropped the message on an element that is not intended for dropping element. Interesting, I think I did not test that 🙈

@irgendwie
Copy link
Member

irgendwie commented Nov 28, 2016

So you've dropped the message on an element that is not intended for dropping element. Interesting, I think I did not test that 🙈

Think of a user, wanting to move the mail, readin it again for a short amount of time and simply doesn't want to move it anymore and releasing the cursor

Edit: Move the cursor (moving mail) to the border of your window -> endless scrolling 😄 @ChristophWurst

@Quix0r
Copy link

Quix0r commented Nov 29, 2016

#202 still lacks code-review (as I can see?).

@jancborchardt jancborchardt mentioned this pull request Nov 30, 2016
@ChristophWurst
Copy link
Member Author

#202 still lacks code-review (as I can see?).

No, it doesn't. I has been reviewed and merged already :-) #202 (comment)

@ChristophWurst ChristophWurst added this to the 0.6.2 milestone Dec 6, 2016
@Quix0r
Copy link

Quix0r commented Dec 6, 2016

Okay. :-)

@jancborchardt
Copy link
Member

Sooo, what's missing here? The message vanish animation and the fix of the floating icon when draghed somewhere else?

@Quix0r did you review this too? :)

@ChristophWurst ChristophWurst modified the milestones: 0.6.2, 0.6.3 Dec 8, 2016
@ChristophWurst
Copy link
Member Author

So you've dropped the message on an element that is not intended for dropping element. Interesting, I think I did not test that 🙈

Tested: mail icon moves back and nothing is moved ✔️

Edit: Move the cursor (moving mail) to the border of your window -> endless scrolling 😄 @ChristophWurst

Confirmed, but I'm unable to find an easy solution. Any idea how we could prevent that?

@ChristophWurst
Copy link
Member Author

Anyway, I decided to do a release before we get this in (0.6.2, #231). Once that is out, I suggest to merge this if this is in a usable state. Any small glitches can be fixed later too 😉

@jancborchardt
Copy link
Member

@ChristophWurst so is this testable or in a usable state so we can merge it? The release was done ;)

@ChristophWurst
Copy link
Member Author

@onny @GuillaumeAmat @jancborchardt this is in a mergable state now IMO. The basic functionality works, I've updated the branch and it works on FF and Chromium. Please give it a test :-)

@ChristophWurst
Copy link
Member Author

coverage/coveralls — Coverage decreased (-0.5%) to 32

Will add some tests when I'm in the mood 🙊 🙉 🙈 probably in another PR

@onny
Copy link
Collaborator

onny commented Dec 17, 2016

So far it works good for me. Some kind of animation is missing though. Further, there is no possibility to move mails with the mobile interface.

@ChristophWurst
Copy link
Member Author

Further, there is no possibility to move mails with the mobile interface.

Yup, we have to think about that too. Drag-and-drop is primarily used for desktop use. We'll have to find another solution for mobile. Suggestions are very welcome :-)

@ChristophWurst
Copy link
Member Author

So far it works good for me. Some kind of animation is missing though.

Known issue. And as stated above, I will work on that in a follow-up PR. Counting your test as positive review and therefore I'm merging this now. Thanks you!

@ChristophWurst ChristophWurst merged commit 3df3fd1 into master Dec 18, 2016
@ChristophWurst ChristophWurst deleted the moving-mails branch December 18, 2016 16:31
@plastikschnitzer
Copy link

plastikschnitzer commented Dec 18, 2016

Concerning moving mails on mobile devices: I have 2 ideas:

  1. Give the possibility to select (multiple) emails and have a dropdown of options like delete, move, mark as …, and others.
  2. Moving mails to the side offers the options from the dropdown options of solution 1 behind the email. Seen in the Samsung Mail app, see my screenshot here, I moved the mail in the middle of the scrreenshot to the side, now I can select the options:
    screenshot_20161218-174638

Best regards,
Plastikschnitzer

@ChristophWurst
Copy link
Member Author

Hey @plastikschnitzer,

thanks for sharing your thoughts on this :-)

Moving mails to the side offers the options from the dropdown options of solution 1 behind the email. Seen in the Samsung Mail app, see my screenshot here, I moved the mail in the middle of the scrreenshot to the side, now I can select the options:

This is a nice gesture, but I'm afraid it's hard to implement it in a web app taking into account that those gestures do not work as fluent in web as on native applications.

Give the possibility to select (multiple) emails and have a dropdown of options like delete, move, mark as …, and others.

I totally agree on this, we have to add that add some point.

We should continue the discussion in a new ticket, otherwise it could get lost. @plastikschnitzer could you please open a new issue and describe your ideas there?

@plastikschnitzer
Copy link

OK, see #255

:-)

@pafcioooo
Copy link

Reading this thread I suppose this functionality is working.
But I am on NC 11 with mail 0.6.2 I I have no clue how to move messages - I tried clicking and holding, right clicking, searching for some buttons...
Please advice.

@MariusBluem
Copy link
Member

MariusBluem commented Feb 22, 2017

Well ... This is merged, but not in a release yet. The milestone for this is 0.6.3 as you can see above.

@pafcioooo
Copy link

@MariusBluem, thank you:-)

@avanc avanc mentioned this pull request May 4, 2017
@ChristophWurst ChristophWurst modified the milestones: 0.6.5, 0.7 Aug 2, 2017
@plastikschnitzer
Copy link

I tested it in the pre-0.7 release – moving mails in between folders of one account works fine. Moving from the combined inbox does not work, the little mail icon remains at the place you dropped the email, so its not obvious to the user what actually happens here. It would be better if the animation would be like the case when you drop the email somewhere where its not allowed (mail symbol moving back to the initial location).

But so far great feature, love it! Makes this app much more usable!

@lock
Copy link

lock bot commented Nov 20, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

@lock lock bot locked and limited conversation to collaborators Nov 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeError: this.replyComposer is undefined Generic moving of messages [$130 awarded]

9 participants