Mobile: Resolves #4989: Implement sorting of notes #7981
Conversation
jcgurango
changed the title
| }; | ||
| })(NoteListComponent); | ||
|
|
||
| export default NoteList; |
There was a problem hiding this comment.
Unfortunately I can't review this, since git shows this as a deleted file and a new file instead of a renamed file. You may have to use the git mv command or similar to make it work. If we can't review we can't merge
There was a problem hiding this comment.
git shows this as a deleted file and a new file instead of a renamed file. You may have to use the git mv command or similar to make it work
Hmm, unfortunately, that's not how git works. Rename detection is a client-side concern, driven by heuristics around the number of lines changed. git mv a b is really just a shortcut for mv a b && git add a && git add b, doing the physical move and adding the removed and added files to the index.
If it is important to you that this be treated as a move in your review, then you must ask that the file be "changed less" (in terms of touched-line-count, across all commits). However, looking at the changes, many of them appear related to the switch to tsx, I'm not sure at a glance which ones are optional.
There was a problem hiding this comment.
Ok I was hoping mv would be part of the heuristic they could use for this, but indeed if it's not recorded anywhere it's annoying. There isn't much that's changed in this file actually, so I'm surprised git cannot detect that it's the same file. Perhaps we should have two pull requests - one for the TS conversion, and one for the actual changes
There was a problem hiding this comment.
Thanks for the explanation @TaoK, I guess perhaps because it's a different file type it's not detected as a rename? I can separate out the TS change.
There was a problem hiding this comment.
I guess perhaps because it's a different file type it's not detected as a rename?
No, the rename detection is based on file change % (by lines). The default change % to trigger a rename detection is 50% - I expect that's what github uses.
There isn't much that's changed in this file actually
I see most of it changed: I can see it as a rename, locally, by telling git to drop the similarity requirement to 10% rather than the 50% default, eg git diff ...jcgurango/feature/mobile-sorting --find-renames=10%, and I then see that almost all the imports are different, the interfaces are new, the declarations are different, much/most of the rendering logic is changed, and the export is different.
jcgurango
force-pushed
the
feature/mobile-sorting
branch
from
212210a to
79cacc4
Compare
|
The drag and drop of notes is nice, but unfortunately we then lose the existing long press. Isn't it possible to keep both? If you long press and move the note, you drag and drop it, but if you don't move you open the menu. I think that's how it's generally handled in mobile apps.
It displays a menu, but the "OK" button seems to be grayed out? |
|
Okay I've kind of combined the two gestures. Now the drag will just start if you've started dragging the item, and the long press is back but won't trigger if you're dragging an item. Not quite sure of the best way to do long-press-then-drag, so if anyone's got suggestions we can try them. Also not sure why your "OK" is greyed out, seems fine on my end: Tested on my actual phone too. |
|
I've been trying to get this working on my laptop but there are so many issues I couldn't. Was it building for you, and did you commit all the changes? For example, I had to add packages to package.json, remove some variables that weren't in use, etc. But even after that, I'm still getting this error: 
And I don't know how to fix this. If you're interested in getting this PR working I put my changes in this branch, including resolved conflicts: https://github.com/laurent22/joplin/commits/jcgurango-feature/mobile-sorting |
Seems the FlatList was going past the screen. Tried putting the flex: 1 on the DraggableFlatList itself but that makes it disappear. Putting it in a View seems to work.
My bad, seems this was the right way.
jcgurango
force-pushed
the
feature/mobile-sorting
branch
from
844f868 to
d8f721c
Compare
|
@laurent22 Rebased and it's building for me now. Turns out my initial implementation for combining the gestures fell apart when scrolling (i.e. when the list doesn't all fit on the screen). Tried several different ways involving a GestureDetector, composing gestures, etc. This was the best I could come up with, I don't think we're gonna be able to get it to be more responsive than this without some extensive reengineering. |
So what is the issue with the current implementation? |
|
@laurent22 it's usable, just that you have to long press and release instead of just long pressing. |
|
@laurent22 Would it be possible to merge this for release 2.12? Manual sorting is a prerequisite for several note taking workflows not yet possible on mobile and certainly a long-awaited and appreciated feature. Even if usability is not optimal yet, it would be great if this would be enabled at all. Thank you for the great work! |
|
@jcgurango, sorry for taking time to review this. I would like to test the change on simulator - would you mind fixing the conflict so that the branch can be built? |
|
@jcgurango, I'm going to close this pull request for now, but feel free to let me know if you'd like to get it working again |
The mobile apps do not currently have the ability to edit the custom sort of notes (even though these are displayed and function correctly if performed first on Desktop and then synchronized). This is pretty necessary for my personal usage of the app so I've implemented it myself and sharing my solution. I'm open to changing how it all works to conform more to the rest of the app if necessary.
Related issue: #4989
Note sorting is implemented by a long-press and drag gesture.
joplin-sorting-demo.webm
If the current sorting is not "Custom Sort", the user is prompted with a message similar to Desktop.
joplin-sorting-message-demo.webm
The original long-press to select is replaced with a swipe gesture. I'm open to making this a setting or something, or maybe we can add a right-swipe menu that includes many actions (delete, duplicate, move, select many).
joplin-sorting-selection-demo.webm