[fix] st.feedback icon wrapping with minimum width enforcement #12970
Conversation
sfc-gh-lwilby
added
security-assessment-completed
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
sfc-gh-lwilby
force-pushed
the
fix/feedback-minimum-width
branch
from
c89cc2f to
243899d
Compare
- Enforce 80px minimum for thumbs (2 icons) - Enforce 200px minimum for faces/stars (5 icons) - Prevents icon wrapping when width is too small - Fixes gh-12068
sfc-gh-lwilby
force-pushed
the
fix/feedback-minimum-width
branch
from
243899d to
2f90fce
Compare
sfc-gh-lwilby
changed the title
sfc-gh-lwilby
force-pushed
the
fix/feedback-minimum-width
branch
from
2277410 to
84ec13e
Compare
Uses theme.baseFontSize to dynamically calculate threshold, making the solution responsive to any custom theme. Fixes gh-12068
sfc-gh-lwilby
force-pushed
the
fix/feedback-minimum-width
branch
from
84ec13e to
210138c
Compare
There was a problem hiding this comment.
question: Seems like these moved down a bit? Any idea why?
There was a problem hiding this comment.
Hmm, good eye. I am not completely sure but I think it has to do with adding new elements above this element since that is the only change in the PR that is related to this test. The width options for this one haven't been affected by this change.
There was a problem hiding this comment.
Ah yeah I've seen it do that before. Frustrating, but I don't know a way around it :(
There was a problem hiding this comment.
Pull Request Overview
This PR fixes icon wrapping issues in st.feedback when the specified width is too small. The solution enforces minimum width thresholds calculated dynamically based on the theme.baseFontSize configuration option. When users specify a width smaller than necessary to fit the icons without wrapping, the widget automatically converts it to width="content" mode.
Key Changes:
- Added minimum width calculation logic in the
feedback()method that computes thresholds based on button/gap sizes in rem units - Added comprehensive Python unit tests covering the width enforcement behavior with different font sizes
- Added E2E snapshot tests to visually verify that small widths are handled correctly
Reviewed Changes
Copilot reviewed 4 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/streamlit/elements/widgets/button_group.py | Implements minimum width enforcement logic using baseFontSize-aware threshold calculations before passing width to the underlying button_group |
| lib/tests/streamlit/elements/button_group_test.py | Adds three new test methods covering width conversion for small values, preservation of adequate widths, and dynamic threshold adjustment with different font sizes |
| e2e_playwright/st_feedback.py | Adds test cases for minimum width enforcement with thumbs and stars feedback widgets |
| e2e_playwright/st_feedback_test.py | Adds snapshot test function to verify visual rendering of minimum width enforcement |
| e2e_playwright/snapshots/linux/st_feedback_test/* | Adds snapshot baseline images for chromium, firefox, and webkit browsers showing thumbs/stars with enforced minimum widths and updated dynamic feedback snapshots |
| # Convert small pixel widths to "content" to prevent icon wrapping. | ||
| # Calculate threshold based on theme.baseFontSize to be responsive to | ||
| # custom themes. The calculation is based on icon buttons sized in rem: | ||
| # - Button size: ~1.5rem (icon 1.25rem + padding 0.125rem x 2) | ||
| # - Gap: 0.125rem between buttons | ||
| # - thumbs: 2 buttons + 1 gap = 3.125rem | ||
| # - faces/stars: 5 buttons + 4 gaps = 8rem | ||
| base_font_size = config.get_option("theme.baseFontSize") or 16 | ||
| button_size_rem = 1.5 | ||
| gap_size_rem = 0.125 | ||
|
|
||
| if options == "thumbs": | ||
| # 2 buttons + 1 gap | ||
| min_width_rem = 2 * button_size_rem + gap_size_rem | ||
| else: | ||
| # 5 buttons + 4 gaps (faces or stars) | ||
| min_width_rem = 5 * button_size_rem + 4 * gap_size_rem | ||
|
|
||
| # Convert rem to pixels based on base font size, add 10% buffer | ||
| min_width_threshold = int(min_width_rem * base_font_size * 1.1) | ||
|
|
||
| if isinstance(width, int) and width < min_width_threshold: | ||
| width = "content" | ||
|
|
There was a problem hiding this comment.
question: Naively, this feels like a frontend problem. Why does it need to be solved in the backend?
There was a problem hiding this comment.
This is a good question.
My thinking is that on the frontend we have the StyledElementContainer layer which is where we apply the styling based on the width sent from the backend.
If we wanted to apply a min width there, it would require a custom carve-out for just this element type. We could apply it further down in the DOM inside the components for button groups as an alternative, but my sense is that this might lead to some issues since the intention is to apply width styling on the StyledElementContainer.
We do have exceptions with elements like charts and dataframes, but those also come with a fair amount of complexity in understanding how the width is working.
I also think it is cleaner to use the content width instead of choosing a specific rem min-width value because it will fit the StyledElementContainer to the icons so we don't need to calculate the exact rem to achieve that effect.
Describe your changes
Prevents st.feedback icons from wrapping when width is too small.
Changes Made:
baseFontSizeconfig option.Below these minimums, we use
width="content"so that the wrapper layout container will hug the icons.GitHub Issue Link
Fixes #12068
Testing Plan
Additional Notes:
When users specify a width smaller than needed for the icons, the widget now enforces the minimum width to prevent wrapping. This follows the same pattern as st.color_picker (gh-12872) by enforcing constraints at the Python API level, preventing layout wrapper size mismatches.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.