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

fix: fix loadSider false render structure issue #2470

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

Koooooo-7
Copy link
Member

@Koooooo-7 Koooooo-7 commented Jul 19, 2024

Summary

Enhance fix #519 .

Main fix

When the loadSidebar=false, it uses the toc to render auto-generated sidebar.
But it puts the children nodes contents out of the parent node <li> block, which is mismatch the behavior with loadSidebar=true.

Minor changes

Refine the sidebar render part to always trigger by _renderMain callback instead of mess it in #renderMain.
Now, the renderSidebar has the consistency that always being a callback after #renderMain no matter it load on loadSidebar=true/false.

Related issue, if any:

What kind of change does this PR introduce?

Bugfix

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?

Yes
No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Copy link

vercel bot commented Jul 19, 2024

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 Jul 27, 2024 7:13am

@jhildenbiddle
Copy link
Member

@Koooooo-7 --

Tested this PR today. The nesting appears to be fixed (🥳), however a bug has been introduced where it appears that an attempt is made to load a sidebar file named null.md even when loadSidebar: true. You can see this by looking at the network tab of your browser's dev tools. The failing e2e test also shows the error message appearing in the console:

       "beforeEach",
        "afterEach-async",
        "afterEach",
        "doneEach",
        "ready",
    +   "Failed to load resource: the server responded with a status of 404 (Not Found)",

@Koooooo-7
Copy link
Member Author

however a bug has been introduced where it appears that an attempt is made to load a sidebar file named null.md even when loadSidebar: true.

@jhildenbiddle ...nice catch. I got a silly mistake to re-load the sidebar twice.
Fixed.

@Koooooo-7 Koooooo-7 added this to the 5.x milestone Jul 20, 2024
@Koooooo-7 Koooooo-7 marked this pull request as ready for review July 22, 2024 16:32
@Koooooo-7 Koooooo-7 requested review from paulhibbitts and a team July 22, 2024 16:32
@Koooooo-7 Koooooo-7 marked this pull request as draft July 22, 2024 16:35
Copy link
Member

@jhildenbiddle jhildenbiddle left a comment

Choose a reason for hiding this comment

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

A few issues I've found:

  1. When loading sidebars from a file (loadSidebar: true), there are now multiple .app-sub-sidebar elements when there should only be one (the <ul> that contains the links generated from page headings). I'm guessing this is because the rendering loop is adding the app-sub-sidebar class to each <ul>element (top-level and each nested list).

  2. When generating sidebars without a file (loadSidebar: false), there is no app-sub-sidebar element. As mentioned above, the <ul> element that links generated from page headings should always have the app-sub-sidebar class name applied.

  3. Separate but related, it appears chore: upgrade marked from 12.0.2 to 13.0.2 #2467 introduced a new behavior that results in the top-level <h1> tag appearing as a nested link. This isn't necessarily wrong, but it is a new and unexpected behavior. I mention it here only because it seems related to the work being done here.

    Docsify v4

    Docsify v5 style overhaul (feat: v5 style overhaul #2469 Preview)

    This PR

test/integration/sidebar.test.js Outdated Show resolved Hide resolved
@Koooooo-7
Copy link
Member Author

Koooooo-7 commented Jul 26, 2024

@jhildenbiddle ---
Fix the issue 1 and 3.
The Issue 2, once we have a final decision I will update asap.
Issue2, aligned to add on the '.sidebar-nav > ul`.

Copy link
Member

@jhildenbiddle jhildenbiddle 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, @Koooooo-7!

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.

Thanks so much @Koooooo-7 , looks good with my testing with Docsify-This using the above Preview build as well.

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.

Fix inconsistent sidebar nesting
3 participants