Desktop: Move note sort drop handler to ItemList to enlarge dropzone downwards #7777
Conversation
|
As far as I can tell there is nothing "wrong" with the change in this pull request, and it's probably good to merge, but I do have a couple of questions for the team about it: Drop handler on note items Is there any practical reason to have had the drop-handler on the note items, rather than the item list? I have changed which element handles the drop event because it is the simplest approach - less codepaths, less confusion - but in principle I could instead have added the itemlist drop handler, and prevented propagation or something to prevent double-firing. I expect the approach I took here is the right one, but I'm asking in case there's something I'm missing with, for example, screen readers: could the existence of a drop handler on note items be a signal to anything? Adding a drop handler to NoteListControls The change proposed here only fixes the bottom dropzone. For drag-drop to work "naturally", the NoteListControls should also be made a dropzone for dropping "above" the first note. In approaching this, I'm not sure how to register a handler in NoteListControls that's defined in NoteList, a "sibling" component. The drop handler I'd like to reference/register in NoteListControls is I don't think it makes sense to move this logic into the shared parent component, NoteListWrapper, but maybe that is indeed the simplest right thing? Any feedback appreciated. |
…downwards Sorting notes was slightly finicky because, to move things to the top or bottom, you needed to drop onto the top half of the top note item in the list, or the bottom half of the bottom note item. This change fixes this for the bottom drop - the dropzone for "to bottom" is enlarged beyond the bottom half of the bottom list item, to the entire remainder of the NoteList (ItemList) div below.
TaoK
force-pushed
the
tao-enlarge-sort-dropzone
branch
from
5d08cf1 to
6dcc62a
Compare
|
Rebased to newer commit to see if automation runs better; failures appeared unrelated to changes. |
|
I don't know, I just feel like the way it currently works is good enough. It's not so difficult to drag the note within the top half or bottom half of the note. The problem is the empty area below the list is not a drop target, so maybe that's what needs to be changed? |
I am highly confident that the current behavior is not good enough, because it simply wasn't working for me, and it took me a while to understand why. I was dragging things to (beyond) the bottom, like I would dropping, and it simply wasn't working. There is definitely a bug / a violation of normal user expectations here, but you can interpret it at least two ways:
I don't understand what you mean... I did change that empty area, by making it part of the drop-zone. That's what this patch in effect does. By having the entire itemlist be the dropzone, the empty area beneath the list items automatically "works". Are you proposing something different? |
| @@ -76,6 +78,10 @@ class ItemList extends React.Component<Props, State> { | |||
| if (this.props.onKeyDown) this.props.onKeyDown(event); | |||
| } | |||
|
|
|||
| onDrop(event: any) { | |||
| if (this.props.onNoteDrop) this.props.onNoteDrop(event); | |||
There was a problem hiding this comment.
This ItemList component is generic, so please could you rename props.onNoteDrop to props.onDrop?
Sorting notes was slightly finicky because, to move things to the top or bottom, you needed to drop onto the top half of the top note item in the list, or the bottom half of the bottom note item.
This change fixes this for the bottom drop - the dropzone for "to bottom" is enlarged beyond the bottom half of the bottom list item, to the entire remainder of the NoteList (ItemList) div below.
This change improves #7776, but does not fix it - the top dropzone is still awkward.