-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix logo pushing out collapse button #12329
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.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
✅ 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 fixes a UI layout issue where the logo was pushing out the collapse button in the Streamlit sidebar. The fix ensures proper positioning and visibility of both elements.
- Added CSS
objectPosition: "left"to prevent logo overflow - Wrapped the logo component in a div container for better layout control
- Enhanced E2E tests to verify collapse button visibility alongside logo display
Reviewed Changes
Copilot reviewed 3 out of 17 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/app/src/components/Sidebar/styled-components.ts | Added objectPosition: "left" CSS property to logo styling |
| frontend/app/src/components/Logo/LogoComponent.tsx | Wrapped logo element in a div container |
| e2e_playwright/st_logo_test.py | Added hover interactions and collapse button visibility checks to all logo tests |
| // handle the width in all cases. It already gets wrapped via a | ||
| // link element (<a>) above when link is provided. | ||
| // https://github.com/streamlit/streamlit/issues/12326 | ||
| return <div>{logo}</div> |
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'm not entirely sure why having this wrapped into another element works fine, but it's probably related to some specific aspect of the surrounding flex box and how max-width is applied to the image.
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.
LGTM 👍🏼
## Describe your changes Fixes an issue with the logo pushing out the collapse button. This was already working fine when a link was set -> the reason is that the additional wrapping leads to a more correct calculation of the actual available width. I think this is also solvable differently with some CSS stuff, but I think its ok to just wrap the logo in a div when its not already wrapped into a `a` element. ## GitHub Issue Link (if applicable) - Closes #12326 ## Testing Plan - Updated the e2e tests to ensure that the collapse button is displayed during snapshots. The logos in the previous version where somewhat broken. --- **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
Fixes an issue with the logo pushing out the collapse button. This was already working fine when a link was set -> the reason is that the additional wrapping leads to a more correct calculation of the actual available width. I think this is also solvable differently with some CSS stuff, but I think its ok to just wrap the logo in a div when its not already wrapped into a
aelement.GitHub Issue Link (if applicable)
st.logocan push minimise button outside sidebar #12326Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.