Preserve scroll position for st.selectbox and st.multiselect
#10073
Conversation
jrieke
changed the title
st.selectbox scroll positionst.selectbox scroll position
jrieke
changed the title
st.selectbox scroll positionst.selectbox and st.multiselect
jrieke
added
security-assessment-completed
jrieke
changed the title
st.selectbox and st.multiselectst.selectbox and st.multiselect
|
This is so much better! Is it possible to open to the last selected option instead of the last view? (I understand if this is a quicker solution (that is clearly an improvement), but I'd expect the scroll to match the last click event rather than a possibly exploratory scroll.) |
|
Hm I'd actually argue it makes more sense at the last scroll position. E.g. if I have option 20 selected and I go through the list looking for something at position 80, and then close the dropdown because I need to look up something else, it would be pretty annoying if it opens at 20 again. (Can see both sides of course but since this is already implemented, will leave it as is.) |
|
Fair enough. In that case, I'd raise a tiny vote to leave #4901 open because it does say "last selected item." The PR wouldn't really do anything for a widget with a default value that's freshly loaded, for example (at least it didn't when I tested the wheel). When I open a picker, I expect to be at the current selection. I would still upvote the feature request to at least deal with the initial scroll position. (The PR is still a huge improvement and I love it, don't get me wrong...) 😁 |
|
Oops, I should properly read the issue description before implementing haha. I changed it to 1) open at the default value if the dropdown hasn't been scrolled before and 2) open at the previously scrolled position if it has been scrolled. |
| component: VirtualDropdown, | ||
| props: { | ||
| $menuListProps: { | ||
| initialScrollOffset: getInitialScrollPosition(), |
There was a problem hiding this comment.
What happens if the initialScrollOffset is larger as the available menu list (for some unexpected reason). Does it run into an error, or will it just scroll to the bottom?
E.g. lets say we allow changing the options for the same widget, would it break the dropdown somehow?
There was a problem hiding this comment.
Just tested it, it simply scrolls to the end, doesn't break.
| // Close dropdown | ||
| // TODO: Utilize user-event instead of fireEvent | ||
| // eslint-disable-next-line testing-library/prefer-user-event | ||
| fireEvent.keyDown(selectbox, { key: "Escape" }) |
There was a problem hiding this comment.
nit: did you try to use user-event here? Did it cause some issues?
There was a problem hiding this comment.
Ah I thought I did, but apparently I just used fireEvent because it was used in the other test above. I tried it out and it works, so I changed this test to userEvent. Unfortunately, I couldn't get the other test above with fireEvent to work with userEvent, so I'm leaving that as is for now.
…eamlit#10073) ## Describe your changes When you click on a selectbox (or multiselect), scroll in the dropdown, then close it and click on the selectbox again, the dropdown opens at the top position. This can be annoying if the dropdown is large. With this PR, the dropdown will open at the same position where it was previously closed. Also, when the dropdown opens for the first time, it opens at the position of the selected default value. **Before:** https://github.com/user-attachments/assets/7b361fb1-0e0d-47db-b627-5391455a2228 **After:** https://github.com/user-attachments/assets/69ca577a-d78e-43fb-8a26-11ec56003638 ## GitHub Issue Link (if applicable) Closes streamlit#4901 ## Testing Plan - Added a JS unit test on the `Selectbox` component that simulates scrolling, closes the dropdown, and checks that it opens at the same position. - Did not add an e2e test because it would basically do the same thing and that feels a bit overkill. But I can add e2e test for `st.selectbox` and `st.multiselect` if we want to have them. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Johannes Rieke <[email protected]>
#10238) ## Describe your changes Revert #10073 because @lukasmasuch and @jrieke detected an issue with it. ## GitHub Issue Link (if applicable) ## Testing Plan - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Any manual testing needed? --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
Describe your changes
When you click on a selectbox (or multiselect), scroll in the dropdown, then close it and click on the selectbox again, the dropdown opens at the top position. This can be annoying if the dropdown is large. With this PR, the dropdown will open at the same position where it was previously closed. Also, when the dropdown opens for the first time, it opens at the position of the selected default value.
Before:
CleanShot.2024-12-31.at.00.55.38.mp4
After:
CleanShot.2024-12-31.at.00.55.03.mp4
GitHub Issue Link (if applicable)
Closes #4901
Testing Plan
Selectboxcomponent that simulates scrolling, closes the dropdown, and checks that it opens at the same position.st.selectboxandst.multiselectif we want to have them.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.