Delay slider thumbValueAlignment to allow page layout to complete
#6828
Conversation
|
Re: E2E Testing this fix: it("realigns label values when expander re-opened", () => {
// Closes the expander
cy.get(".streamlit-expanderHeader").click();
// Reopens the expander
cy.get(".streamlit-expanderHeader").click();
// matchImageSnapshot used to test initial thumb value positioning
// theme change interactions allow for fix to error in initial position
cy.getIndexed(".streamlit-expander", 0).matchImageSnapshot();
});Note: A snapshot test could prove flaky due to the timing inconsistency of the thumb realignment and how long it takes to capture the snapshot - if thats the case an alternative option is to test the CSS resulting from realignment - this seemed to work (fail without fix, pass with fix): it("realigns label values when expander re-opened", () => {
// Closes the expander
cy.get(".streamlit-expanderHeader").click();
// Reopens the expander
cy.get(".streamlit-expanderHeader").click();
// Positioning error occurs on overflow of expander container
// which occurs when position left set to 0px
cy.getIndexed(".StyledThumbValue", 5).should("not.have.css", "left", "0px")
});Let me know if you run into any issues, happy to pair! 😃 |
|
Thanks, Maya! I was out sick later last week until today, so I didn't make any updates in the interim. On 38c5608, I copied your option to test the CSS resulting from the realignment for both Please lmk if there's any other changes required for this PR, including more test coverage. |
e2e/scripts/st_select_slider.py
Outdated
| @@ -88,3 +88,7 @@ def on_change(): | |||
| ) | |||
| st.write("Value 8:", st.session_state.select_slider8) | |||
| st.write("Select slider changed:", "select_slider_changed" in st.session_state) | |||
|
|
|||
| with st.expander("Expander", expanded=True): | |||
| r2 = st.slider("Label 9", 10000, 25000, [10000, 25000]) | |||
There was a problem hiding this comment.
Nit:st.select_slider with relevant params vs. st.slider
There was a problem hiding this comment.
Oops, good catch! Fixed on bffef9a
There was a problem hiding this comment.
@snehankekre just the one comment on e2e test fix, but otherwise LGTM 👍🏼
Also my fault for interpreting the TODO as a question vs your notes 😅
|
All good! You've saved me a lot of time writing the tests 😸 |
* develop: Minimum version dependency testing (streamlit#6802) Delay slider `thumbValueAlignment` to allow page layout to complete (streamlit#6828)
…treamlit#6828) * Delay slider thumbValueAlignment to allow page layout to complete * Update st.slider and st.select_slider e2e tests * Update num of slider elements in select slider e2e * use st.select_slider in select slider e2e * Fix session state init error * Update index on StyledThumbValue snapshot to 11
…treamlit#6828) * Delay slider thumbValueAlignment to allow page layout to complete * Update st.slider and st.select_slider e2e tests * Update num of slider elements in select slider e2e * use st.select_slider in select slider e2e * Fix session state init error * Update index on StyledThumbValue snapshot to 11
…treamlit#6828) * Delay slider thumbValueAlignment to allow page layout to complete * Update st.slider and st.select_slider e2e tests * Update num of slider elements in select slider e2e * use st.select_slider in select slider e2e * Fix session state init error * Update index on StyledThumbValue snapshot to 11
Describe your changes
#5913 fixed sliders to adjust the label if it appears outside of the screen, and for ranges where there are two thumb values. That solution is 99% of the way there. But as #6297 demonstrates, it doesn't consider how the slider and select_slider widgets behave when the expander is expanded and collapsed multiple times.
My best guess of the observed behavior: When the expander is collapsed, the sliders are hidden and their layout may be invalidated. Upon re-expansion, the thumb values might be painted before the container sets its final width.
This PR fixes this by using
setTimeout(() => { ... }, 0), which delays the execution of thethumbValueAlignmentmethod until the sliders are fully rendered with their final widths. This ensures correct alignment and prevents the thumb values from overflowing in subsequent expander expansions.GitHub Issue Link (if applicable)
Closes #6297.
Testing Plan
- TODO: e2e tests need to be updated. I need to figure out how to get cypress to expand -> collapse -> re-expand anst.expanderwith nested sliders before taking a snapshot.st_slider.spec.js,st_select_slider.spec.js, andst_select_slider.py.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.