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

Clicking on an empty space in the area will cause pixel offset #208

Closed
Hweiya opened this issue Oct 8, 2023 · 5 comments · Fixed by #210
Closed

Clicking on an empty space in the area will cause pixel offset #208

Hweiya opened this issue Oct 8, 2023 · 5 comments · Fixed by #210

Comments

@Hweiya
Copy link

Hweiya commented Oct 8, 2023

Describe the bug
Clicking on an empty space in the area will cause pixel offset

To Reproduce
Steps to reproduce the behavior:

  1. Create a project to import the dragselect component
  2. Write the area property, giving a specific width and height and overflow:auto
  3. Click on the empty space in the area
  4. When there are enough child elements to support the height, it is obvious that the scroll bar has been rolled down some distance, which is not the desired effect

Expected behavior
I want it to stay still

Code Example
1696752089515

Screenshots
If applicable, add screenshots to help explain your problem.
1696752009026

Desktop (please complete the following information):

  • OS: window
  • Browser chrome
  • Version dragselect": "^2.7.4"

Additional context
Add any other context about the problem here.

@dbougan
Copy link
Contributor

dbougan commented Oct 15, 2023

@Hweiya @ThibaultJanBeyer
I ran into this issue as well and think I found out what is causing it. The canScroll method seems to manually adjust the scrollTop property of the drag area to check if it is scrollable: https://github.com/ThibaultJanBeyer/DragSelect/blob/main/DragSelect/src/methods/canScroll.ts#L13-L18

I don't know enough to say if that's the best way to verify scrollability, but I think we could implement the simple change shown in this diff. It performs the same operation, but restores the scrollTop property to it's original value before the DOM is updated.

image

@ThibaultJanBeyer
Copy link
Owner

Hey everyone, thanks for using the tool!! @dbougan, feel free to issue a PR with your proposal and we get it merged as a workaround and add a @todo "find a better way to test scrollability". This gives us time to figure out if there is not a better way to test it. Unless you also feel like researching this immediately of course.
Thanks again for looking into this and providing a working alternative 😊

@dbougan
Copy link
Contributor

dbougan commented Oct 15, 2023

Hey @ThibaultJanBeyer, I created a new Pull Request #210 with this change and a new test case to cover it
Thanks for your responsiveness, and please let me know if you would like anything changed in the PR

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Oct 18, 2023

Fixed in 3.0.4, thanks a lot @dbougan <3
You’re added to the pool of contributors

@dbougan
Copy link
Contributor

dbougan commented Oct 18, 2023

I'm happy to help out @ThibaultJanBeyer, thanks for pulling it in!

Also, I'm sorry to see about the test failure I caused. Apparently, during development, I was running yarn in a way that only ran tests related to my code changes. I see the correct way to run all tests now and will do that going forward. But thank you for sorting out the Settings tests!

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 a pull request may close this issue.

3 participants