Make Default Sidebar Width Configurable #12154
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. |
|
Hi @jose-mindwayai , Thanks for submitting this PR! At a first glance, it looks good. Before we review/merge PRs we need product approval that the feature is something we want to add. @jrieke is out this week but he will take a look when he gets back. |
|
Hi @sfc-gh-lwilby, thanks for the update! Sounds good 👍 |
|
Hey @jose-mindwayai! Thanks for the contribution. From your issue, it seems like the only problem you have is that the new sidebar width is too small for you, but you were happy with the old one? In that case, I'm thinking whether we should just revert to the old width... I'm not a huge fan of introducing a theme option for this, since I think most people will be fine with the default width (we just need to make sure it's a good default!). Can you by any chance share some screenshots of how your app/sidebar looks with the old and new widths? |
|
Hi @jrieke, thanks for getting back to me! 🙂 Yes, you’re right — the main issue I ran into is that the new sidebar width feels a bit too narrow compared to the old one. I didn’t have any problems with the previous width, so reverting would definitely solve it for my use case. That said, I think having an option may be useful since sidebar content can vary a lot between applications. This is a screnshot of the old preset which was set at 336px. This is a screnshot of the new preset which is set at 256px. I think the sidebar contents feel a little squished because the filter controls don’t have enough horizontal breathing room. Text like “Avg. session time” wraps more tightly and some labels feel crowded against the edges. 
|
|
Hey, sorry for the delay! We're discussing internally whether we should revert that back to the old sidebar width now. As I mentioned above, I'd much prefer this over adding a config option/parameter for it. |
|
No worries! That makes sense 👍 |
|
Hi @jrieke have you got any updates? :) |
|
Hey @jose-mindwayai! So we are fine with changing the sidebar width again. As I said before, I think I agree that the 256px width was a bit too narrow. I've been just playing around and I was thinking maybe we should do 300px by default, that's a nice in-between value that's a bit smaller than the old one but not too narrow. Then your sidebar above would show up like this on a 15 inch notebook, what do you think? 
I think I'd be in principle even fine with having a parameter for the default width. The question is just where to put it. Theming feels a bit awkward since this isn't really part of a "theme" to make your app follow a different design system. One pretty clever thing we could do is fold this into the |
|
Hey @jrieke really cool that you took the time to reproduce the issue! 🙂 I agree that 300px seems like a good default for my use case, definitely a nice middle ground. That said, I also think it would be great to give users the option to customize the sidebar width. Should I try implementing it that way? |
|
Yup, if you have the time for it, please go ahead! And you can also include the change to 300px in the same PR. Let me know when I should have a look :) |
875d891 to
13a6875
Compare
|
Hey @jrieke I think this should work now 👍 Let me know what you think :) |
|
Awesome! On a conference at the moment, so might take a bit, but trying to have a look next week! |
|
@jrieke I've also added the new snapshots now :) |
|
Hey @jrieke sorry to bug you again, just wondering if you got any updates? |
|
Hey, no worries, thanks for the ping, we had a lot going on! This seems to work fine, the only issue I found is that if you put in a really large value (e.g. Lmk as soon as that is fixed and then I'll get our engs to have a look! |
Thanks @jrieke I hadn't noticed that 😅 I have now limited the width between 200 and 600px. Everything seems to be working now! |
|
I am also interested in setting the width of the sidebar because I want to to create a fixed-width bar to display icons only. Does the min-width need to be clamped at 200? For an icon bar, I might want something like 64 or 128. Random image for reference: |
This reverts commit 4ef301c9dc97cd48cf8b540a81de1eee61685427.
42ceed5 to
8f00964
Compare
| const [sidebarWidth, setSidebarWidth] = useState<string>(() => { | ||
| // Use initialSidebarWidth if available, otherwise fall back to cached or default | ||
| if (notNullOrUndefined(initialSidebarWidth)) { | ||
| const clampedWidth = clampSidebarWidth(initialSidebarWidth) | ||
| return cachedSidebarWidth || clampedWidth.toString() | ||
| } | ||
| return cachedSidebarWidth || DEFAULT_WIDTH | ||
| }) |
There was a problem hiding this comment.
The cached sidebar width from localStorage is not being clamped, which could result in invalid width values being applied on initial load. If a user manually edits localStorage or if the cached value predates the min/max constraints, the sidebar could render with an invalid width (e.g., 1000px which exceeds the 600px maximum).
The fix should clamp the cached width before using it:
const [sidebarWidth, setSidebarWidth] = useState<string>(() => {
const getCachedWidth = () => {
if (cachedSidebarWidth) {
const cached = parseInt(cachedSidebarWidth, 10)
return isNaN(cached) ? null : clampSidebarWidth(cached).toString()
}
return null
}
const clampedCached = getCachedWidth()
if (clampedCached) {
return clampedCached
}
if (notNullOrUndefined(initialSidebarWidth)) {
return clampSidebarWidth(initialSidebarWidth).toString()
}
return DEFAULT_WIDTH
})| const [sidebarWidth, setSidebarWidth] = useState<string>(() => { | |
| // Use initialSidebarWidth if available, otherwise fall back to cached or default | |
| if (notNullOrUndefined(initialSidebarWidth)) { | |
| const clampedWidth = clampSidebarWidth(initialSidebarWidth) | |
| return cachedSidebarWidth || clampedWidth.toString() | |
| } | |
| return cachedSidebarWidth || DEFAULT_WIDTH | |
| }) | |
| const [sidebarWidth, setSidebarWidth] = useState<string>(() => { | |
| const getCachedWidth = () => { | |
| if (cachedSidebarWidth) { | |
| const cached = parseInt(cachedSidebarWidth, 10) | |
| return isNaN(cached) ? null : clampSidebarWidth(cached).toString() | |
| } | |
| return null | |
| } | |
| const clampedCached = getCachedWidth() | |
| if (clampedCached) { | |
| return clampedCached | |
| } | |
| if (notNullOrUndefined(initialSidebarWidth)) { | |
| return clampSidebarWidth(initialSidebarWidth).toString() | |
| } | |
| return DEFAULT_WIDTH | |
| }) | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
@jose-mindwayai this seems like a good idea if you can address this prior to merging.
|
@jose-mindwayai heads up there were some merge conflicts in your branch so I have rebased on develop for you and force pushed the changes. |
|
@jose-mindwayai this PR looks good to me! The only thing is this graphite review comment above, this seems like a tood thing to address before merging because it could perhaps lead to some user experience issues. I've completed the rebase and fixed up the branch for you including a force push after rebasing so heads up on that. |
|
Hi @sfc-gh-lwilby thank you very much for the help :) I've added graphite's suggestion and created some test cases for that as well 👍 |
sfc-gh-lwilby
left a comment
There was a problem hiding this comment.
LGTM thank you @jose-mindwayai
|
Great, thanks for the review @sfc-gh-lwilby! |
|
Wohoo, thanks for getting this across the finish line, @jose-mindwayai! 🥳 Would love to send you some swag as a little thank you, just fill out this form and we'll get it on the way: https://forms.gle/RZCUTKCrg7aHyqH78 |
|
Thanks a lot @jrieke, I really appreciate it 😄 |
Describe your changes
initial_sidebar_stateinst.set_page_config()parameter to accept an integer value for setting initial sidebar width in pixels.The initial_sidebar_state parameter in st.set_page_config() now accepts:
GitHub Issue Link (if applicable)
Ability to Set Default Sidebar Width #11980
Testing Plan
I did some manual testing and added Unit Tests for the sidebar width validation.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.