Skip to content

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
@vercel
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 28, 2024 8:15am
@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 requested a review from jhildenbiddle July 20, 2024 08:49
@Koooooo-7 Koooooo-7 marked this pull request as ready for review July 22, 2024 16:32
@Koooooo-7 Koooooo-7 requested review from sy-records and trusktr July 22, 2024 16:32
@Koooooo-7 Koooooo-7 requested review from a team and paulhibbitts July 22, 2024 16:32
@Koooooo-7 Koooooo-7 marked this pull request as draft July 22, 2024 16:35
@Koooooo-7 Koooooo-7 marked this pull request as ready for review July 24, 2024 12:33
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 #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 (#2469 Preview)

    This PR

@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.

@Koooooo-7 Koooooo-7 merged commit 7cbd532 into develop Jul 28, 2024
@Koooooo-7 Koooooo-7 deleted the fix-loadsidebar branch July 28, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants