-
Notifications
You must be signed in to change notification settings - Fork 194
Sort dictionaries by drag and drop v2 #1970
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
base: master
Are you sure you want to change the base?
Conversation
Used a previous pull request code as a base for this. However, previously the code had efficiency issues. So I will look for a workaroudn for this in a future commit.
…'t do the more process intensive call while dragging.
|
Thanks for the PR! I've tried it and it works. It's kind of hard to tell where the dropped dict will get sorted. Here I try to put the 1st dict in between dicts 2 and 3, but it goes to 3: It would also be nice if the dictionaries got scrolled when trying to drag out of the box, but not sure if this is a must-have: And ideally, we would have the elements move so you always know exactly where it will end up when dropped: |
logic for choosing where to drop dictionary
Used a previous pull request code as a base for this. However, previously the code had efficiency issues. So I will look for a workaroudn for this in a future commit.
…'t do the more process intensive call while dragging.
logic for choosing where to drop dictionary
|
I think I fixed some of the issues you mentioned with the previous commits. @StefanVukovic99 I could move the logic of updating the sorting to happen |
Kuuuube
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things from a quick look:
- Is is possible to get this working on mobile? It seems to just do nothing when I tested it there.
- It would be nice if when dragging there could be a highlighted line that shows the position the dict is going to be put. (we already have separator lines so maybe a color change/bold sort of thing there)
- Is there any way to cancel after dragging? Hitting esc or right clicking just drops it normally instead of canceling.
- It might be a good idea to style the currently dragging entry differently. Stefan mentioned actually having it move positions as you drag. But I think for now even something as simple as lowering the opacity would be good so it's clearer which dict has been picked up.
|
A quick update:
|
|
Am I correct in understanding that the current implementation in this PR allows the entire dictionary row area to trigger |
Looks great.
This is good but I think it would be more intuitive to only have the border on the top. That way only the part where it's going to "slot in" is shown and it's unambiguous whether it's going to go in above or below.
Considering right clicking or hitting esc cancels a drag on nearly every other program that has dragging I think we probably need that here. It's not particularly nice to have the item dropped where it is instead of canceling the operation when either of these are pressed. Also I'm in agreement on needing to constrain the area for dragging. Just using the arrow up/down icon youve added should be fine for the zone. |
Mentioned this in the commits. But this is work to allow users to short their dictionaries using drag and drop.
Based on the nature of dragover we can't really stop it from firing while in a valid drop area. So the second best thing is using the
preventDefault()function in thedragoverhandler and putting most of the logic on the drop which will happen once. That should be more efficient.This is my first contribution so any feedback would be helpful. I typically use React and typescript so the OOP coding paradigm took some adjustment. Fixes: #796