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

sidebar uses native <details> for collapsible groups [#3804. also #3517, #3805] #3806

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

olets
Copy link
Contributor

@olets olets commented Apr 18, 2024

Had time to hack on this now, so didn't wait to hear if it was desirable. Close if you need to ๐Ÿ‘

Closes #3804

Fixes #3517, closes #3802 as obviated

Fixes #3805

3804 is the primary target of this PR. Preserving the behaviors identified in 3517 and 3805 would have taken additional work, so I took the opportunity to address them for "free".

Before

Sidebar collapsible groups used custom markup and JS. Carets are focusable, an accessibility problem.

After

  • 3804: Sidebar collapsible groups use native <details> elements. Custom markup and JS is reduced.
  • 3517: dropped the separately-clickable carets. See PR interactive controls are not nested [#3517]ย #3802 for details on rationale.
  • 3805: Clicking linked text in a collapsible group's heading doesn't toggle the group.

To test

  1. With the released version, replace docs/.vitepress/config/en.ts's sidebarGuide with the following.

    function sidebarGuide(): DefaultTheme.SidebarItem[] {
      return [
        {
          text: 'Introduction',
          collapsed: true,
          items: [
            {
              text: 'Group heading linked to "What is VitePress?"',
              link: 'what-is-vitepress',
              collapsed: true,
              items: [
                {
                  text: 'Another group heading linked to "What is VitePress?"',
                  link: 'what-is-vitepress',
                  collapsed: true,
                  items: [
                    {
                      text: 'What is VitePress?',
                      link: 'what-is-vitepress',
                    },
                  ],
                },
              ],
            },
            { text: 'Getting Started', link: 'getting-started' },
            { text: 'Routing', link: 'routing' },
            { text: 'Deploy' }
          ]
        },
        { text: 'What is VitePress?', link: 'what-is-vitepress',},
        {
          text: 'Writing',
          collapsed: false,
          items: [
            { text: 'Markdown Extensions', link: 'markdown' },
            { text: 'Asset Handling', link: 'asset-handling' },
            { text: 'Frontmatter', link: 'frontmatter' },
            { text: 'Using Vue in Markdown', link: 'using-vue' },
            { text: 'Internationalization', link: 'i18n' }
          ]
        },
        {
          text: 'Customization',
          collapsed: false,
          items: [
            { text: 'Using a Custom Theme', link: 'custom-theme' },
            {
              text: 'Extending the Default Theme',
              link: 'extending-default-theme'
            },
            { text: 'Build-Time Data Loading', link: 'data-loading' },
            { text: 'SSR Compatibility', link: 'ssr-compat' },
            { text: 'Connecting to a CMS', link: 'cms' }
          ]
        },
        {
          text: 'Experimental',
          collapsed: false,
          items: [
            { text: 'MPA Mode', link: 'mpa-mode' },
            { text: 'Sitemap Generation', link: 'sitemap-generation' }
          ]
        },
        { text: 'Config & API Reference', base: '/reference/', link: 'site-config' }
      ]
    }
  2. Run pnpm run docs

  3. Open http://localhost:<your port>/guide/what-is-vitepress in a browser

  4. Confirm that the "Introduction" group is closed. [3804 regression check]

  5. Toggle "Introduction" and its children open by clicking on the carets [3517]. Confirm that all items linked to "What is VitePress?" have the active color. [3804 regression check]

  6. Toggle unlinked group headings, eg. "Writing". Confirm that they toggle when either the text or caret is clicked. [3517]

  7. Confirm that the non-group item linked to "What is VitePress?" has the indicator. [3804 regression check]

  8. Toggle "Introduction" > "Group heading linked to โ€œWhat is VitePress?โ€" closed.

  9. Confirm that hovering over "Introduction" > "Deploy", which is not linked, does not give it the hover color. [3804 regression check]

  10. Confirm that hovering of the text "Group heading linked to โ€œWhat is VitePress?โ€" gives it the hover color. [3804]

  11. Confirm that hovering with the cursor over linked sidebar items gives them the hover color. [3804 regression check]

  12. Click "Introduction" > "Routing"

  13. Under "Introduction", click the text of the first item, "Group heading linked to "โ€œWhat is VitePress?โ€". Confirm that the site navigates and the group stays closed. [3805]

@olets olets marked this pull request as ready for review April 18, 2024 06:55
@olets
Copy link
Contributor Author

olets commented Apr 18, 2024

(sorry for that noise. almost never resolve merge conflicts via the github UI and forgot it would merge the base into the feature. disregard the "'main' into sidebar-details", it's reverted)

Copy link
Member

Choose a reason for hiding this comment

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

.item:hover .caret and .item:hover .caret:hover styles should probably be merged and adjusted to .item summary:hover .caret { color: var(--vp-c-text-1) } (because currently item is whole element instead of just that title part)

@brc-dd
Copy link
Member

brc-dd commented Apr 26, 2024

Also, regarding #3805 I don't think that's a bug. If I am on this state:

image

I would want clicking next page will uncollapse the "Writing" section in sidebar.

@brc-dd
Copy link
Member

brc-dd commented Apr 26, 2024

Also, the tests seem to be failing, please update those too.

@brc-dd brc-dd added the needs author action The PR is not ready yet label Apr 26, 2024
@olets
Copy link
Contributor Author

olets commented Apr 26, 2024

Thanks for the feedback. Been meaning to say that I see the failing tests and will look at them. Will try to find time for this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs author action The PR is not ready yet
Projects
None yet
2 participants