-
Notifications
You must be signed in to change notification settings - Fork 108
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
feat: added axis limitation option #593
Open
Tormak9970
wants to merge
5
commits into
isaacHagoel:master
Choose a base branch
from
Tormak9970:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ec9a08d
feat: added axis limitation option
Tormak9970 79d9671
chore: fixed warning about ignored prop
Tormak9970 7c2872e
fix: ensured keyboard dragging now accounts for axis config
Tormak9970 0cd9527
fix: added screen reader alerts if an axis is disabled
Tormak9970 8e3b60d
chore: fixed missing config param
Tormak9970 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
as you can see in the original code, arrow right is exactly like arrow down, it moves the item from index i to index i+1 if possible, without concerning itself with where the items are placed in space.
To achieve the axis limitation might be tricky.
It can potentially be achieved by checking bounding client rects relative to the original position of the dragged item but i think even that won't work in some edge cases
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.
I see. Perhaps it makes sense to only have the limit work for pointer drag, since its mainly useful for mobile devices
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.
Let's take a step back for a minute. What is the problem that limiting the drag axis is trying to solve from a UX perspective?
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.
At least for the use cases I've seen, there are often times where you're only using drag and drop for reorderability. Here's the example that led me to make this PR:
I have a music player, and both the queue and playlist views have a reorderable list of songs. I want it to be clear that there's no purpose to dragging other than changing the order, so limiting it to only the y-axis indicates that.
It's also nice on mobile devices in the case where your fingers may move slightly and it keeps the element in line with the list its in
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.
Just thinking out loud here... If the feature was to only "allow dragging" in a direction that has other items you could displace, would that achieve the same goal? In the image you included the user can still drag upwards and leave the drop zone, with this implementation they will be able to drag up until the items becomes the first in the list and then moving the mouse further up (or to the side) won't take it further up (same way that locking to the axis doesn't let it move to the sides)
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.
Yea that would effectively achieve the same UX. I don't see a use case for one and not the other. Would be more work to implement tho I think
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.
Yeah it would be harder to implement but would be in line with the lib supporting any special arrangement of items as it does now (e.g. grid, board etc).
If you're keen I am happy to brainstorm, otherwise I can take a crack at it but not sure when I will have time (maybe over the weekend). There will need to be a util that tracks positions of items or something like that
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.
I'm happy to take a crack at it, there may be a relatively straightforward solution by using the indices of the items. I'm also interested in potentially converting the library to TS to make editing/maintenance easier but that would cause major conflicts with open PRs
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.
I like TS and there were attempts at this in the past but because of all the dom manipulation it always ended up as not adding much safety.