-
Notifications
You must be signed in to change notification settings - Fork 4k
[AdvancedLayouts] Add height to st.plotly_chart #12593
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!
|
9b9cc6b to
0d3bb6c
Compare
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.
I can't reproduce this change locally in chrome, firefox or safari, but it seems persistent in CI. With that in mind, it seems likely a minor impact to users if it is reproducible outside of CI.
0a8bff4 to
d6b0dce
Compare
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0200%
✅ Coverage change is within normal range. |
69f6ebc to
1c0aef3
Compare
1c0aef3 to
4e24a57
Compare
| ) | ||
| st.plotly_chart(fig, theme="streamlit") | ||
|
|
||
| # Width parameter tests |
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.
I had to split up these tests because otherwise the tests were timing out consistently in CI.
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.
Question: This change seems a little strange to me - any idea why the movement in the time selection buttons?
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.
Nvm - just noticed your comment above
Describe your changes
Adds a new
heightparameter tost.plotly_chartusing theHeighttype system ("stretch","content", or pixel values) for consistency with other chart elements.Changes Made:
height: Height = "stretch"parameterheight="content", respects plotly.Figure's native height if specifiedKey Implementation Details:
Height Parameter Addition:
heightparameter with defaultheight="stretch"height="content"extracts height from native chart object if available, falls back to"stretch"GitHub Issue Link (if applicable)
Testing Plan
Screenshots/Demos:


Additional Notes:
This extends the AdvancedLayouts initiative by adding height support to
st.plotly_chart, bringing it in line with other chart elements that support theHeighttype system.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.