Skip to content

fix: fix loadSider false render structure issue #2470

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 29 commits into from
Jul 28, 2024
Merged

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 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 paulhibbitts and a team 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 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

@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
9 checks passed
@Koooooo-7 Koooooo-7 deleted the fix-loadsidebar branch July 28, 2024 08:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix inconsistent sidebar nesting
3 participants