-
Notifications
You must be signed in to change notification settings - Fork 4k
Add gaps and sizes for xxsmall, xsmall, xlarge, xxlarge in st.container/columns/space #13345
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
Conversation
✅ 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!
|
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.
Pull request overview
This PR extends the gap and size options for st.container, st.columns, and st.space by adding four new settings: "xxsmall" (0.25rem), "xsmall" (0.5rem), "xlarge" (6rem), and "xxlarge" (8rem). The smaller sizes address the need for tighter spacing when laying out widgets, while the larger sizes provide symmetry and enable separating major app sections.
Key changes:
- Extended the GapSize protobuf enum with four new values (XXSMALL, XSMALL, XLARGE, XXLARGE)
- Updated Python backend type definitions, validation, error messages, and documentation
- Added frontend spacing values (fiveXL, sixXL) and updated gap translation logic
- Refactored E2E tests to comprehensively cover all gap/size options using loops
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/streamlit/proto/GapSize.proto | Adds four new gap size enum values to support the extended size range |
| lib/streamlit/elements/lib/layout_utils.py | Updates type aliases, SIZE_TO_REM_MAPPING, validation logic, and gap_mapping to include new sizes |
| lib/streamlit/elements/space.py | Updates docstring to document the new xxsmall, xsmall, xlarge, and xxlarge size options |
| lib/streamlit/elements/layouts.py | Updates docstrings for st.container and st.columns to document the new gap options |
| lib/streamlit/errors.py | Updates error message to include all seven valid gap size options |
| lib/tests/streamlit/elements/space_test.py | Adds parameterized test cases for the four new space sizes |
| lib/tests/streamlit/elements/lib/layout_utils_test.py | Adds parameterized test cases for new sizes in validation and config tests |
| frontend/lib/src/theme/primitives/spacing.ts | Adds fiveXL (6rem) and sixXL (8rem) spacing values |
| frontend/lib/src/components/core/Block/styled-components.ts | Updates translateGapWidth switch statement to handle all new gap sizes |
| e2e_playwright/st_space.py | Refactors test to loop through all space sizes including new ones |
| e2e_playwright/st_layouts_container_gap_size.py | Refactors test to loop through all gap sizes for both horizontal and vertical containers |
| e2e_playwright/st_columns.py | Refactors test to loop through all gap sizes for column layouts |
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0000%
✅ Coverage change is within normal range. |
e2e_playwright/st_space_test.py
Outdated
| for height_em in heights_em: | ||
| height_px = round(height_em * 16) | ||
| space = space_elements.nth(0) | ||
|
|
||
| wait_until(app, wait_for(space, height_px)) |
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.
The test loop always checks space_elements.nth(0) (the first space element) for all height values, which will cause test failures. The loop should iterate through different space elements using an incrementing index.
Fix:
for i, height_em in enumerate(heights_em):
height_px = round(height_em * 16)
space = space_elements.nth(i)
wait_until(app, wait_for(space, height_px))| for height_em in heights_em: | |
| height_px = round(height_em * 16) | |
| space = space_elements.nth(0) | |
| wait_until(app, wait_for(space, height_px)) | |
| for i, height_em in enumerate(heights_em): | |
| height_px = round(height_em * 16) | |
| space = space_elements.nth(i) | |
| wait_until(app, wait_for(space, height_px)) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
SummaryThis PR adds four new gap/space size options ( Key changes:
Code QualityThe code is well-structured and follows existing patterns in the codebase:
Test CoverageExcellent test coverage across all layers: Python Unit Tests
TypeScript Unit Tests
E2E Tests
The E2E tests follow best practices:
Backwards CompatibilityThis PR is fully backwards compatible:
Security & RiskNo security concerns identified. These are purely additive visual/layout changes:
Regression risk is low:
RecommendationsNo blocking issues. A few minor observations:
VerdictAPPROVED: This is a well-implemented feature addition with excellent test coverage, clean code, and full backwards compatibility. The new gap sizes address a real user need for finer-grained layout control. This is an automated AI review. Please verify the feedback and use your judgment. |
mayagbarnes
left a comment
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.
Couple comments but overall LGTM 👍🏼
…er/columns/space (streamlit#13345) ## Describe your changes The `SIZE` in `st.container(gap=SIZE)`, `st.columns(gap=SIZE)`, `st.space(SIZE)` now supports smaller and larger settings:`"xxsmall"`, `"xsmall"`, `"xlarge"`, and `"xxlarge"`. The smaller settings are important because the smallest today is `1rem`, which is too large for many use cases, especially when laying out widgets next to each other. The larger settings are less important, but they're here mostly for symmetry -- though I _could_ see them being useful to separate large sections of an app. One weird thing: the existing `SIZE` settings for `st.space` do not match those for `st.container`/`st.columns`. There are reasons for that, but it's still somewhat funky. For this work I didn't touch any of this. But I also didn't contribute to making the problem larger. Insteade I just ignored it altogether and made the new `st.space` sizes match the ones I'm adding for `st.container`/`st.columns`. Wishlist: If we didn't care about breaking changes, I'd love to make the default gap be `medium` rather than `small`, and make `medium` mean `1rem` (same as today's `small`). But we'll have to fight that fight some other day! ## Screenshot or video (only for visual changes) See e2e snapshot updates. ## GitHub Issue Link (if applicable) n/a ## 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
The
SIZEinst.container(gap=SIZE),st.columns(gap=SIZE),st.space(SIZE)now supports smaller and larger settings:"xxsmall","xsmall","xlarge", and"xxlarge".The smaller settings are important because the smallest today is
1rem, which is too large for many use cases, especially when laying out widgets next to each other. The larger settings are less important, but they're here mostly for symmetry -- though I could see them being useful to separate large sections of an app.One weird thing: the existing
SIZEsettings forst.spacedo not match those forst.container/st.columns. There are reasons for that, but it's still somewhat funky. For this work I didn't touch any of this. But I also didn't contribute to making the problem larger. Insteade I just ignored it altogether and made the newst.spacesizes match the ones I'm adding forst.container/st.columns.Wishlist: If we didn't care about breaking changes, I'd love to make the default gap be
mediumrather thansmall, and makemediummean1rem(same as today'ssmall). But we'll have to fight that fight some other day!Screenshot or video (only for visual changes)
See e2e snapshot updates.
GitHub Issue Link (if applicable)
n/a
Testing Plan
Explanation of why no additional tests are neededAny 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.