-
Notifications
You must be signed in to change notification settings - Fork 15
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: Do not overscroll the Teams filter #3951
Fix: Do not overscroll the Teams filter #3951
Conversation
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.
Hahahah I am sorry for this <3
But thanks for the simple fix, nice one 😎
Tried scrolling, dragging, everything seems to work as intended 👌
arrow-overscroll-2024-12-16_20.34.46.mp4
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.
Nice catch, @rumzledz 😄
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.
Nice start on this issue @rumzledz 🙌
However, I still experience some issues if I select Andromeda
for eg. and then try to scroll left
Screen.Recording.2024-12-18.at.22.04.45.mov
I'm leaving my thoughts on how we should maybe refactor the scrollLeft
and possibly also the scrollRight
functions to properly work
For scrollLeft
, we need to ensure the scroll value stays within the available scrollable space. Since -clientWidth * 0.5
could exceed the remaining scrollable space, we should first calculate the remaining space. If the remaining space is smaller than clientWidth * 0.5
, we use the remaining space as the scroll offset instead of scrolling by half the width of the container.
Thus this should do the trick
const scrollLeft = () => {
if (!containerRef.current) {
return;
}
const { clientWidth, scrollLeft: currentScrollLeft } = containerRef.current;
const boundedScrollLeft = Math.min(clientWidth * 0.5, currentScrollLeft);
containerRef?.current.scrollBy({
left: -boundedScrollLeft,
behavior: 'smooth',
});
};
Pretty similar logic also for the scrollRight
- which I know is not intended in this PR, but I did notice the right overscroll
const scrollRight = () => {
if (!containerRef.current) {
return;
}
const {
clientWidth,
scrollLeft: currentScrollLeft,
scrollWidth,
} = containerRef.current;
const remainingScroll = scrollWidth - currentScrollLeft - clientWidth;
const boundedScrollLeft = Math.min(clientWidth * 0.5, remainingScroll);
containerRef?.current.scrollBy({
left: boundedScrollLeft,
behavior: 'smooth',
});
};
🙌
dc07e16
to
329501f
Compare
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.
Thanks for the changes @rumzledz 🥇 I'll approve given I've already tested them 💯
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.
Another day, another dollar
Another year, another overscroll issue solved 😎
overscroll-fix-2025-01-07_15.35.02.mp4
🚢
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.
Perfect! All working very nicely.
Description
The left scroll's distance multiplier was somehow causing the team filter to shift way beyond the boundaries so I set it to match the right scroll's distance multiplier,
0.5
.Testing
Note
I don't know any other way for anyone to test this apart from doing this in a Simulator
master
branch and redo steps 2 to 4Resolves #3540