Skip to content
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

Implement reorderable layers by mouse #281

Merged
merged 24 commits into from Feb 17, 2020
Merged

Conversation

barklund
Copy link
Contributor

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?

@swissspidy
Copy link
Collaborator

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
Contributor

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.

@barklund
Copy link
Contributor Author

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.

Copy link
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
Collaborator

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
Contributor Author

barklund commented Feb 12, 2020

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
Collaborator

Let‘s defer speak() to later.

I‘ll try to reproduce the other thing

@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
@BrittanyIRL BrittanyIRL mentioned this pull request May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants