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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scroll): add possibility to disable auto scrolling #2178

Conversation

VincentMolinie
Copy link
Contributor

@VincentMolinie VincentMolinie commented Jan 21, 2023

Description

Right now the scroll is way too fast. To prevent that I've implemented my own system of scrolling, but I need the possibility for the scroll to be disabled to make it work correctly 馃槄

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

@adumesny
Copy link
Member

adumesny commented Jan 21, 2023

thank for adding this option - but if it's really too fast, maybe it would be better to tweak the current code instead ?
there are also 1 or 2 bugs related to scrolling not working so would be great to fix as well...

@adumesny adumesny merged commit 82e81fa into gridstack:master Jan 21, 2023
@VincentMolinie VincentMolinie deleted the feat/add-possibility-to-disable-auto-scroll branch January 22, 2023 16:06
@VincentMolinie
Copy link
Contributor Author

Thanks a lot for merging this 馃檹

@VincentMolinie
Copy link
Contributor Author

VincentMolinie commented Jan 23, 2023

Hey @adumesny,
Could you please release a minor 馃懠 ?

@adumesny
Copy link
Member

I will once I get a couple fixes... lots of overhead to release

@adumesny
Copy link
Member

actually this is where it should have been implemented (what jqeury-ui did) instead of adding a top option per grid

/** Drag&Drop dragging options */
export interface DDDragOpt {
  /** class selector of items that can be dragged. default to '.grid-stack-item-content' */
  handle?: string;
  /** default to 'body' */
  appendTo?: string;
  /** if set (true | msec), dragging placement (collision) will only happen after a pause by the user. Note: this is Global */
  pause?: boolean | number;
  /** default to `true` */
  // scroll?: boolean;
...

Can you please revert your change and updae the doc as well ? thanks.
But I would MUCH prefer fixing the root cause than adding a 'bandaid' - I see #2188 (I don't beleive seeing that on windows so maybe a mac only issue ? I haven't looked...

@VincentMolinie
Copy link
Contributor Author

I moved the option here: #2192

The problem with the scroll is that we scroll of a difference between the bottom of the element and the bottom of the screen which might be huge, we should use maybe something like:

    const MAX_SCROLL = 100;
    if (top) {
      // This also can be done with a timeout to keep scrolling while the mouse is
      // in the scrolling zone. (will have smoother behavior)
      scrollEl.scrollBy({ behavior: 'smooth', top: Math.max(-MAX_SCROLL, pointerPosY - distance)});
    } else if (bottom) {
      scrollEl.scrollBy({ behavior: 'smooth', top: Math.min(MAX_SCROLL, distance - (height - pointerPosY))});
    }

But even though, the scroll is not really good at the moment as you need to move your mouse to scroll, if your mouse just stay in the scrolling zone, it doesn't scroll 馃.

A way better solution, it's what I implemented on my side, should be to add dragging zone on each side and on mouse enter start scrolling, on mouse exit or drop we stop the scroll

@adumesny
Copy link
Member

adumesny commented Feb 1, 2023

can you please contribute your fix to this lib instead ? I didn't write the scroll part so open to re-writting it out...

@adumesny
Copy link
Member

@VincentMolinie I would appreciate getting better scrolling in the lib if you can move your custom version here instead...

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

2 participants