Skip to content
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

feat: allow collapsible sidebar items #663

Merged
merged 3 commits into from
May 26, 2022
Merged

feat: allow collapsible sidebar items #663

merged 3 commits into from
May 26, 2022

Conversation

brc-dd
Copy link
Member

@brc-dd brc-dd commented May 26, 2022

fixes #640

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Thanks a lot! As always, I'll adjust a bit on styles since I actually have the design for this toggle feature 😅

But before doing that, I would like to discuss a bit on config option!

First, let's make collapsible -> collapsable, just because VuePress config is this way 👀

Next, I would like to have defaults to open/closed option. So collapsable: true means sidebar group is collapsable. But I think it would be helpful to have option to set if the collapsable group is open by default, or closed by default.

So how about we add 2 options?

export interface SidebarGroup {
  text: string
  items: SidebarItem[]

  // If `false`, toggle button is hidden.
  // Default: true
  collapsible?: boolean

  // If `false`, collapsible group is collapsed by default
  // Default: true
  open?: boolean
}

What do you think?

@brc-dd
Copy link
Member Author

brc-dd commented May 26, 2022

First, let's make collapsible -> collapsable, just because VuePress config is this way 👀

Its collapsible in their docs: https://v2.vuepress.vuejs.org/reference/default-theme/config.html#sidebar

So how about we add 2 options?

Fine.

Also, lets keep collapsible false by default. VuePress also has the same.

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Oh sorry for that. I was looking at VuePress v1 doc 😓 Let's go with collapsible 👍

Also, lets keep collapsible false by default. VuePress also has the same.

Gotcha. Sounds perfect 👍

@brc-dd
Copy link
Member Author

brc-dd commented May 26, 2022

Changed the interface to this:

  export interface SidebarGroup {
    text: string
    items: SidebarItem[]
    // If `true`, toggle button is shown.
    // Default: false
    collapsible?: boolean
    // If `true`, collapsible group is collapsed by default.
    // Default: false
    collapsed?: boolean
  }

There is a certain issue with current implementation. When a group is collapsed by default, how do we get the items height for smooth transition? Currently, I am setting collapsed on mount, which is causing a flash on reload.

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Changed the interface to this:

Nice! I like collapsed much better 👍

There is a certain issue with current implementation. When a group is collapsed by default, how do we get the items height for smooth transition? Currently, I am setting collapsed on mount, which is causing a flash on reload.

I was thinking to not have smooth animation and just pop open/close 😅 I think it's not possible (at least in an easy way). So I would say for now go with no animation... until we find out some cool technique.

Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

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

I've swapped the icon and adjusted the styles.

I went with no style for now. I think we can animate it by using FLIP technique, but it's a bit of work and we can always add it later 👍

@kiaking kiaking merged commit e32936b into vuejs:next-theme May 26, 2022
@brc-dd
Copy link
Member Author

brc-dd commented May 26, 2022

@kiaking You removed props.collapsible checks too. Now, even if someone hasn't set collapsible (or has set false), one can just click the title to toggle.

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Woops, will fix right away 👍

@brc-dd
Copy link
Member Author

brc-dd commented May 26, 2022

This should fix that:

const collapsed = ref(props.collapsible && props.collapsed)

function toggle() {
  if (!props.collapsible) return
  collapsed.value = !collapsed.value
}

Also, one will be able to always click the title as you've set role="button". Perhaps make it dynamic? Since role is button, these styles aren't needed:

.VPSidebarGroup.collapsible .title {
  cursor: pointer;
}

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Ah... good point. Hmmm, yeah let's only add role="button" also when assigning collapsible too.

@kiaking
Copy link
Member

kiaking commented May 26, 2022

Fixed! 🎉 Thank you so much for your review! 🙏

@brc-dd
Copy link
Member Author

brc-dd commented May 26, 2022

One last one 😅: These horizontal borders are not aligned when scrollbar is shown:

image

Setting this (inside src\client\theme-default\components\VPSidebar.vue:116), should fix this:

@media (min-width: 960px) {
  .group {
    width: calc(var(--vp-sidebar-width) - 64px);
  }
}

@brc-dd brc-dd deleted the feat/collapsible branch May 26, 2022 17:58
@kiaking
Copy link
Member

kiaking commented May 27, 2022

Good catch! Thanks 😳 I'll push the fix now 👍

@hyp530
Copy link

hyp530 commented Jun 17, 2022

When a user enters a specific page, the corresponding SidebarGroup should open automatically (with other SidebarGroups collapsed), and even scroll to the exact sidebar location. Otherwise, a user would definitely get lost in the sidebar, when there're a lot of pages.

Wondering if you could add this feature? @kiaking

@brc-dd
Copy link
Member Author

brc-dd commented Jul 11, 2022

@hyp530 Yeah we can add that. Let's track it at #952

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants