fix(sidebar): remove button to close on mobile#127
Conversation
🦋 Changeset detectedLatest commit: 127ac40 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Tchekda
left a comment
Tchekda
left a comment
There was a problem hiding this comment.
Code looks good but I am not sure to understand why you don't want to collapse the sidebar on mobile ?
I would actually want to collapse it on mobile and force it on desktop ?
|
My idea is to either display or not the sidebar, not just collapsing it. However I agree that it might be very project-specific and other people using that component might not want it to use it this way, so I can rework it to pass it as a prop? |
|
@CptnJon what do you think ? |
| 'flex shrink-0 flex-col items-start whitespace-nowrap transition-all', | ||
| isSidebarOpen | ||
| ? `ml-4 ${props.isGroupOpen ? 'w-fit' : 'w-48'} opacity-100` | ||
| ? `ml-4 w-fit min-w-48 opacity-100` |
There was a problem hiding this comment.
What is the goal of that change? It will change the current behavior that subitems of groups will not have a fixed width of 12rem.
There was a problem hiding this comment.
Having a fixed width and a long title or description in the sidebar item outside a group will cause them to be cut off
There was a problem hiding this comment.
Yeah, I see. I am not sure whether I like a sidebar with 440px width either. Text-Wrapping looks shit as well. @wolfmaster8 do you have a (good) opinion?
|
Sorry, it's late. Let's start over: |
jcalor
changed the title
|
@jcalor I think the best change would be to update our sidebar component to the latest version on shadcn as it handles in a better way mobile shadcn Sidebar. I'll start working on it |
4 participants
We don't want to collapse the sidebar on mobile because it wouldn't be UX friendly.
Mobile looking sidebar: