-
Notifications
You must be signed in to change notification settings - Fork 5
Feature: Global and App sidebar collapse update #2234
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
> | ||
<IconButton |
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.
We want to keep this as a button for accessibility reasons. At the very least we need to give it role="button" and make sure it can be tabbed focused into
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 don't really like this approach. I don't see the need for TWO separate instances of AppSidebarButton. It seems like the only need for a second one is for a slightly different position but
-
This button should not shift. I have questioned @zcolah on this previously and he agreed that they are indeed in the same vertical position.
-
Even if it did need to shift I can see this still being handled and contained within ResizableContainer
@agalin920 I actually tried doing this by placing it inside ResizableContainer but the button seems to get cut off by the global sidebar when the app sidebar collapses. Setting the z-index didn't seem to have fixed it but I'll try to redo it and see if I can make it work. Actually I found out what's causing the issue now. I'll re-write this to simplify. |
@theofficialnar I managed it with overflow: "visible" on SubApp container Since that button is in fact overflowing out of the container onto the |
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.
@theofficialnar (got it right this time I hope :P)
Great job as always. Near perfect as expect. Just a few very tiny issues.
See here: https://www.figma.com/file/zOI7oSH3fG1XDmmPzsP6PX/App-Navigation?type=design&node-id=651%3A104351&mode=design&t=pp22sLin1J6qchZd-1
In the content sidebar please make the padding for the hidden items titlebar to be 12 px on left and 12px on the right. Currently it is 16px. Please do this for the media and schema sidebar. |
Please add a hover state to the hamburger menu icon. When hovered over it should have a background of: |
…sidebar-collapse-update
…sidebar-collapse-update
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.
Preview
collapse.webm