Skip to content

Implement reorderable layers by mouse#281

Merged
swissspidy merged 24 commits intomasterfrom
feature/51-layer-panel-drag
Feb 17, 2020
Merged

Implement reorderable layers by mouse#281
swissspidy merged 24 commits intomasterfrom
feature/51-layer-panel-drag

Conversation

@barklund
Copy link
Copy Markdown
Contributor

@barklund barklund commented Feb 11, 2020

This completes the work in #51 by implementing mouse-reorderable layers.

Accessibility is tricky at the moment though.

There will not be keyboard-access to the reordering, so that's not the problem (it'll be a global reorder shortcut, that they'll use), but mouse users still need to be informed for the potential drop targets as well as the result of the reordering. I've currently added some speak() when dragging about potential drop targets, but not much more than that.

Suggestions?

@barklund barklund self-assigned this Feb 11, 2020
swissspidy
swissspidy reviewed Feb 11, 2020
View reviewed changes
@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Feb 11, 2020

Isn't this kind of reordering with indicators/separators what DropZoneProvider is for? Is it possible to re-use that one somehow / or why not?

As for speak(), that one seems to make more sense when using keyboard shortcuts and not when dragging using the mouse and seeing indicators/separators and the dragged element's clone.

@dvoytenko
Copy link
Copy Markdown
Contributor

dvoytenko commented Feb 11, 2020

Isn't this kind of reordering with indicators/separators what DropZoneProvider is for? Is it possible to re-use that one somehow / or why not?

As we went through the UX review of this feature, there are some UX requirements that made native DND inapplicable. Specifically dragging only within the bounds/axis. I'm afraid that will also mean that we will have to revisit page carousel/grid once we are done with layers.

dvoytenko
dvoytenko suggested changes Feb 11, 2020
View reviewed changes
@barklund
Copy link
Copy Markdown
Contributor Author

barklund commented Feb 11, 2020

Isn't this kind of reordering with indicators/separators what DropZoneProvider is for? Is it possible to re-use that one somehow / or why not?

I couldn't see how this fit in with regular reordering. We don't want keyboard accessibility for it (we do that elsewhere) and we aren't actually dragging anything anywhere. This custom approach seemed much simpler to use here.

As for speak(), that one seems to make more sense when using keyboard shortcuts and not when dragging using the mouse and seeing indicators/separators and the dragged element's clone.

I have no idea if screen readers are only for keyboard users. I would assume there are mouse users who don't see well, so want to have things read out loud to them, but I have no idea. I'd gladly remove it again, as it was a bit weird.

dvoytenko
dvoytenko approved these changes Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of threads are still ongoing, but the PR itself is LGTM.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Feb 12, 2020

As we went through the UX review of this feature, there are some UX requirements that made native DND inapplicable. Specifically dragging only within the bounds/axis. I'm afraid that will also mean that we will have to revisit page carousel/grid once we are done with layers.

Thanks for clarification! I was not aware of that, so that's good to know.

@barklund barklund changed the base branch from feature/51-layer-panel-a11y to master February 12, 2020 20:30
@barklund
Copy link
Copy Markdown
Contributor Author

barklund commented Feb 12, 2020 •
edited
Loading

This PR currently has two open issues, that I don't really know how to address:

  1. Maybe sometimes in random situations, you cannot correctly drag layers. I haven't been able to reproduce, so can't fix it.
  2. Do we want the speak() thing or not? It's a very partial solution as is (doesn't speak what you're about to do, doesn't speak about the result).

Open for thoughts here!

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Feb 12, 2020

Let‘s defer speak() to later.

I‘ll try to reproduce the other thing

spacedmonkey
spacedmonkey reviewed Feb 13, 2020
View reviewed changes
@swissspidy swissspidy merged commit df1acb8 into master Feb 17, 2020
@swissspidy swissspidy deleted the feature/51-layer-panel-drag branch February 17, 2020 12:43
@swissspidy swissspidy mentioned this pull request Feb 17, 2020
Closed
@BrittanyIRL BrittanyIRL mentioned this pull request May 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy approved these changes

@miina miina Awaiting requested review from miina

+2 more reviewers

@spacedmonkey spacedmonkey spacedmonkey left review comments

@dvoytenko dvoytenko dvoytenko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@barklund barklund

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@barklund @swissspidy @dvoytenko @spacedmonkey @googlebot