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: scrolling inside the table with scrollbar #9753

Merged

Conversation

chandraguptgosavi
Copy link
Contributor

Fixed- when scrolling inside the table with scrollbar, cursor also selects rows. Now on scrolling with scrollbar content is scrolled in respective direction as expected and table rows don't get selected.

fixes issue #6773

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Implemented a fix to prevent unintended row selection when scrolling the table using the scrollbar by tracking the scrollbar drag state.

  • Added isScrollHandlerDragging state in /packages/twenty-front/src/modules/ui/utilities/scroll/components/ScrollWrapper.tsx to track scrollbar interaction
  • Updated ScrollWrapperContextValue type in /packages/twenty-front/src/modules/ui/utilities/scroll/contexts/ScrollWrapperContexts.tsx to include drag state
  • Added drag state check in /packages/twenty-front/src/modules/object-record/record-table/components/RecordTable.tsx to disable row selection during scrolling
  • Implemented mouseDown/mouseUp handlers to accurately track when user is dragging the scrollbar

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 108 to 111
if (!isScrollHandlerDragging) {
resetTableRowSelection();
toggleClickOutsideListener(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider moving this condition check into a useCallback to avoid recreating the function on every render

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @chandraguptgosavi !

While this PR addresses the records selection issue, it doesn't prevent the blue selection box from appearing during drag operations.

I suggest we leverage the existing data-select-disable attribute handling that's already implemented in the DragSelect component. The DragSelect component is designed to check for this attribute and automatically prevents selection when it's present. This would be a more consistent approach with our existing codebase.

Here's a suggested implementation:

useEffect(() => {
const currentRef = scrollableRef.current;
if (currentRef !== null) {
initialize(currentRef);
// Add data-select-disable to scrollbars
const scrollbars = currentRef.querySelectorAll('.os-scrollbar');
scrollbars.forEach((scrollbar) => {
if (scrollbar instanceof HTMLElement) {
scrollbar.dataset.selectDisable = 'true';
}
});
}
// ... rest of cleanup code
}, [initialize, instance, setScrollTop, setOverlayScrollbars]);

This approach uses the built-in selection prevention mechanism and keeps the logic close to where the scrollbars are managed.

@chandraguptgosavi
Copy link
Contributor Author

@ehconitin I have made the requested changes.

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing the suggested change! After further investigation, I realized there's an even better way to handle this using the OverlayScrollbars updated event callback.

Instead of using useEffect, we can set the data-select-disable attribute in the updated event, which ensures it's applied whenever the scrollbars are created/updated:

events: {
updated: (osInstance) => {
const { scrollbarVertical, scrollbarHorizontal } = osInstance.elements();
if (scrollbarVertical) {
scrollbarVertical.track.dataset.selectDisable = 'true';
}
if (scrollbarHorizontal) {
scrollbarHorizontal.track.dataset.selectDisable = 'true';
}
// ... existing update logic
}
}

@charlesBochet wdyt? thoughts?

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice lgtm thank you

@martmull martmull enabled auto-merge (squash) January 23, 2025 15:03
@martmull martmull merged commit 1d7cbca into twentyhq:main Jan 23, 2025
45 checks passed
DeepaPrasanna pushed a commit to DeepaPrasanna/twenty that referenced this pull request Jan 27, 2025
Fixed- when scrolling inside the table with scrollbar, cursor also
selects rows. Now on scrolling with scrollbar content is scrolled in
respective direction as expected and table rows don't get selected.

fixes issue twentyhq#6773

---------

Co-authored-by: nitin <[email protected]>
Co-authored-by: ehconitin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants