Skip to content

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

Merged
merged 21 commits into from
Aug 30, 2023

Conversation

finnar-bin
Copy link
Contributor

  • Updated Global sidebar collapse/uncollapse
  • Added collapse/uncollapse for in-app sidebar

Preview

collapse.webm

@finnar-bin finnar-bin requested a review from agalin920 August 18, 2023 05:41
@finnar-bin finnar-bin self-assigned this Aug 18, 2023
>
<IconButton
Copy link
Contributor

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

Copy link
Contributor

@agalin920 agalin920 left a 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

  1. This button should not shift. I have questioned @zcolah on this previously and he agreed that they are indeed in the same vertical position.

  2. Even if it did need to shift I can see this still being handled and contained within ResizableContainer

@finnar-bin
Copy link
Contributor Author

finnar-bin commented Aug 18, 2023

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

  1. This button should not shift. I have questioned @zcolah on this previously and he agreed that they are indeed in the same vertical position.
  2. 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.

@agalin920
Copy link
Contributor

agalin920 commented Aug 18, 2023

@theofficialnar I managed it with

overflow: "visible" on SubApp container

Since that button is in fact overflowing out of the container onto the <aside>

Copy link

@zcolah zcolah left a 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

@zcolah
Copy link

zcolah commented Aug 21, 2023

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.

@zcolah
Copy link

zcolah commented Aug 21, 2023

Please add a hover state to the hamburger menu icon. When hovered over it should have a background of:
Shades/Primary Shades/8p (#FF5D0A14)

Copy link

@zcolah zcolah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tooltips for the following buttons:

  1. For the re-order icon: "Reorder Pages"
  2. For the plus icon: "Create Page"

image

@zcolah
Copy link

zcolah commented Aug 24, 2023

Please add tooltips for the following buttons.

  1. Hide Content
  2. Add Content Item

CleanShot 2023-08-24 at 12 02 45@2x

@zcolah zcolah added the vqa VQA is complete and approved label Aug 25, 2023
@finnar-bin finnar-bin added the ready PR is complete and ready for deployment label Aug 28, 2023
@shrunyan shrunyan merged commit 9dfcea9 into master Aug 30, 2023
@shrunyan shrunyan deleted the feature/sidebar-collapse-update branch August 30, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready PR is complete and ready for deployment vqa VQA is complete and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants