fix(select): keep the scrolling position when multiple select true #3065
Conversation
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3065/ |
|
I tested and unfortunately the new changes brought "the jump" back, so the scrolling position got reset. I will keep my initial commit. |
LucyChyzhova
force-pushed
the
limel-select-multiple-items-avoid-top-scrolling
branch
from
7590138
to
abc8288
Compare
|
The current solution doesn't really fix the issue. In my testing it seemed like at times there were two separate elements being scrolled at the same time. |
src/components/select/select.tsx
Outdated
| // Use setTimeout to restore scroll position after a slight delay | ||
| setTimeout(() => { | ||
| list.scrollTop = scrollPosition; | ||
| }, delay); |
There was a problem hiding this comment.
Using any delay other than 0 introduces a race condition. Either this works with a delay of 0, or we can't rely on this solution as a fix, because it depends on a race condition. (Although, in this case, if it usually works, and only sometimes doesn't, that's a better behaviour for the user than the current behaviour.)
I assume you've already tried this with a delay of 0, but I'll try it myself just in case, and if it doesn't work, I might play around and see if I can get it to work in some other way.
There was a problem hiding this comment.
(Although, in this case, if it usually works, and only sometimes doesn't, that's a better behaviour for the user than the current behaviour.)
I agree!
I assume you've already tried this with a delay of 0, but I'll try it myself just in case, and if it doesn't work, I might play around and see if I can get it to work in some other way.
Thank you, Adrian! 🙏
There was a problem hiding this comment.
Please help test my fixup.
It's a pity that John isn't here to help test it, since he's the one who was having problems with the previous implementation… 😕
There was a problem hiding this comment.
I am actually reading the thread because I'm curious to see if your fixup will solve it. When I still had the issues, it was when I had selected multiple items, but it wasn't consistent. I tried the requestAnimationFrame but didn't try the setTimeout within the rAF as you did. :D
I'll test it now. It's too hot to be outside so I'm in the living room with a fan on full blast and reading through issues in the cooldown board 😆
adrianschmidt
force-pushed
the
limel-select-multiple-items-avoid-top-scrolling
branch
from
abc8288
to
e3ac9d8
Compare
There was a problem hiding this comment.
The current solution doesn't really fix the issue. In my testing it seemed like at times there were two separate elements being scrolled at the same time.
Hm… I'm not seeing anything like that on my machine, and it's working well for me (although with a slight flicker sometimes, but that's not the end of the world).
I wonder if the behaviour you are seeing might be related to the previously mentioned race condition created by setTimeout, or if it's something else… 🤔
There was a problem hiding this comment.
Hm… I'm not seeing anything like that on my machine, and it's working well for me (although with a slight flicker sometimes, but that's not the end of the world).
Same for me 🤔
There was a problem hiding this comment.
Ah! Yeah, there is clearly something weird going on in Edge on Windows:
Screen.Recording.2024-07-29.at.14.23.12.mov
(I have tried in Chrome on macOS, where I'm not having any problems. I haven't tried any other browsers yet.)
There was a problem hiding this comment.
In Edge on macOS it also has odd behaviours.
Screen.Recording.2024-07-29.at.14.33.48.mov
There was a problem hiding this comment.
I think the problem is that we've added the overflow stuff here: https://github.com/Lundalogik/lime-elements/pull/3065/files#diff-817817636ecff6902e33d23a4566f3ce0c76c4b8c1604695c95b7acc3c40b066R30-R33
I'm pretty sure that the drop down could already overflow and become scrollable, before this PR. And since we haven't changed anything related to that existing functionality, that's still in effect. For some reason, that is getting triggered in Edge. My initial suspicion would be that it happens on systems where scroll bars are taking up space, like they do on Windows. I'm not having any problems in Edge Version 127.0.2651.74 (Official build) (arm64) on my Mac, so I'm going to venture a guess that you have your Mac set to always show scroll bars, @john-traas?
There was a problem hiding this comment.
I think I've solved it. Pushing a fixup in a minute.
There was a problem hiding this comment.
YESS! Works everywhere for me! 💥
There was a problem hiding this comment.
Yup, I've tested it in Chrome, Edge, and Safari on my Mac, and Edge on Windows 11. All work flawlessly as far as I can see 🥳
There was a problem hiding this comment.
I'll squash and push 👍
adrianschmidt
force-pushed
the
limel-select-multiple-items-avoid-top-scrolling
branch
from
79fe717
to
8a168d6
Compare
| return; | ||
| } | ||
|
|
||
| const scrollPosition = list.scrollTop; | ||
| const scrollPosition = menuSurface.scrollTop; |
There was a problem hiding this comment.
ah!! I should not use list there! but menuSurface
and .mdc-menu-surface and not limel-menu-surface
There was a problem hiding this comment.
Look at the whole fixup. That's "the same" variable, I just renamed it, because I changed so that it wasn't holding the list, but the menu-surface.
There was a problem hiding this comment.
Here's what the fixup looked like before I squashed it:
There was a problem hiding this comment.
right, missed the top part.
adrianschmidt
force-pushed
the
limel-select-multiple-items-avoid-top-scrolling
branch
from
a7c88a4
to
dbacd80
Compare
|
🎉 This PR is included in version 37.55.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix: #1268
fix: https://github.com/Lundalogik/crm-feature/issues/2808
Screen.Recording.2024-07-15.at.14.48.14.mov
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: