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
Feature: st.logo
#8554
Feature: st.logo
#8554
Conversation
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.
Overall looks good. I have some questions and some places where clarity can be written.
e2e_playwright/__snapshots__/linux/hello_app_test/hello_app-animation_demo_page[chromium].png
Outdated
Show resolved
Hide resolved
...dataframe_interactions_test/st_data_editor-selected_row_for_deletion[dark_theme-firefox].png
Outdated
Show resolved
Hide resolved
...st_dataframe_interactions_test/st_data_editor-row_deletion_toolbar[light_theme-chromium].png
Outdated
Show resolved
Hide resolved
e2e_playwright/__snapshots__/linux/st_graphviz_chart_test/st_graphviz-fullscreen[chromium].png
Outdated
Show resolved
Hide resolved
..._plotly_chart_test/st_plotly_chart-container_width_false_fullscreen[dark_theme-chromium].png
Outdated
Show resolved
Hide resolved
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.
Proto changes LGTM 👍
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.
nit: A couple of the caption test snapshots are a bit cut off (probably overlayed by the header). But I think this might be solved once we update the caption tests to playwright.
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.
nit: it might be a good idea to add a print-test where there is a logo (with sidebar opened and closed).
In general I think it'd be great if we put screenshots into the PR descriptions with a before/after comparison to allow for a quick visual overview for the reviewer of what exactly has changed in a PR |
## Describe your changes The app header bar height changed in #8554 and now the heading can be overlayed by it when clicking on the anchor. This PR fixes this by using the correct height. ## GitHub Issue Link (if applicable) ## Testing Plan - manual tests --- **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
Add a new command
st.logo
to display a logo in the top left of main page & sidebarTesting Plan
Please note: this PR has SO MANY snapshot updates as a result of changing Streamlit's
headerHeight
- this was a Design decision to ensure the header's toolbar buttons align with the logo image & the open/close sidebar buttons. Essentially every sidebar/full screen snapshot is impacted by these changes.