Skip to content

refactor: optimize toggleSidebar trigger conditions #2557

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 1 commit into from
Jun 14, 2025

Conversation

sy-records
Copy link
Member

Summary

When there is no sub sidebar, the sidebar should be closed.

Related issue, if any:

Close #2556

What kind of change does this PR introduce?

Refactor

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Copy link

vercel bot commented Jun 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 12, 2025 8:07am

@paulhibbitts
Copy link
Collaborator

paulhibbitts commented Jun 12, 2025

Hi @sy-records , thanks so much for looking into this!

Your fix looks really good, but before proceeding I thought I'd share two of those original demos updated with your preview build to confirm that everyone is on-board re: updated behaviour.

Here is the updated codebox (using your new build), tap on the second page topic and sidebar now closes on mobile:
https://codesandbox.io/p/sandbox/docsify-v5-sidebar-nav-ljxvyd

Here is an the updated Docsify v5 rc-1 example using your new build - tap on Topic One and Topic two when on mobile and now the sidebar closes as expected:
https://hibbitts-design.github.io/preview-docsify-open-publishing-starter-kit-v2-docsify-v5-theme/#/

Thanks again, rc-1 is really coming along 🚀

@paulhibbitts
Copy link
Collaborator

paulhibbitts commented Jun 13, 2025

I've done more tests and this continues to look solid, I'll add my review now.

I've also updated my test Docsify-This v2 build to use your updated Sidebar docsify.min.js for continued testing, and you can try it out at https://preview-v2.docsify-this.net/?basePath=https://github.com/raw/hibbitts-design/docsify-this-multiple-page-open-publishing-site/main&homepage=home.md&sidebar=true&loadSidebar=_sidebar.md&docsify-core-theme=true#/

Copy link
Collaborator

@paulhibbitts paulhibbitts left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@sy-records sy-records merged commit 71ac41e into docsifyjs:develop Jun 14, 2025
8 checks passed
@sy-records sy-records deleted the fix/2556 branch June 14, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On mobile, the Sidebar remains open when a page with no sub-topics is tapped.
2 participants