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

Fix: selection box in scrollable parent container #36

Closed
wants to merge 2 commits into from

Conversation

deafnv
Copy link

@deafnv deafnv commented Apr 11, 2023

Had an issue with selection in a scrollable parent container where the selection wouldn't select following the cursor if the container was being scrolled while selecting, see: https://codesandbox.io/s/staging-silence-q87qdx

Wasn't able to fix it without changing the package so I thought I'd submit a PR, not entirely sure if I'm missing anything here.

Fixed: https://codesandbox.io/s/red-wildflower-hvlwim

@tomaszczura
Copy link
Collaborator

Thanks for putting this PR! However, I think this won't work if DragSelection is deeply nested, and it's parent is not actual scrolling element. The element that scrolls should be the same that provides events - configured in eventsElement. I played a bit with codesandbox you provided - I moved DragSelection into elements-container and changed eventsElement to be

eventsElement: document.getElementById("elements-container"),

and now selection comes with cursor when I scroll elements container. You just need to adjust logic for selecting elements.

Please let us know if that solution works for you or you still need some changes

@deafnv
Copy link
Author

deafnv commented Apr 18, 2023

As I understand it, the issue you mentioned can be fixed without changing my fork? I tried to replicate the issue you mentioned, and the solution does work. Either way, I'm fine with any solution that fixes the main issue.

@tomaszczura
Copy link
Collaborator

Correct - I think we don't need any changes to the library, because scroll element is different on different pages - it should be always eventsElement. Let us know if you see any bugs when using proposed solution

@deafnv
Copy link
Author

deafnv commented Apr 18, 2023

Looks like it works, though it seems like I'm having issues that could be related with #30, but that's a different matter entirely.

@deafnv deafnv closed this Apr 18, 2023
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.

2 participants