Conversation

YiiGuxing

Summary

The doneEach hook function for the cover should be called after the cover page HTML has been appended to the DOM.

Related issue, if any:

What kind of change does this PR introduce?

Feature

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

The `doneEach` hook function for the cover should be called after the cover page HTML has been appended to the DOM.
@vercelVercel

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

NameStatusPreviewCommentsUpdated (UTC)
docsify-preview✅ Ready (Inspect)Visit Preview💬 Add feedbackJun 5, 2024 2:17pm

@YiiGuxingYiiGuxing marked this pull request as ready for review May 13, 2024 15:33
@jhildenbiddle

Thanks of this, @YiiGuxing. Can you add some tests to test/e2e/plugins.test.js to verify the new behavior? Thx!

@YiiGuxing

@jhildenbiddle. Completed. Please review.

@YiiGuxing

@jhildenbiddle, I found that this PR does not work in the asynchronous mode of marked.js. I am fixing it, please wait for my .

@YiiGuxingYiiGuxing marked this pull request as ready for review May 30, 2024 12:37
@YiiGuxing

@jhildenbiddle, The fact that the cover doesn't work in asynchronous mode on marked.js isn't due to this PR, it's the same problem on the develop branch: setting markdown: { async: true } will cause the cover and the sidebar to malfunction. The new commit ensures that the cover works properly, but there are still issues with the sidebar. The new commits also add support for embedding files in the cover and fix an issue where the doneEach hook might not be called.

Additionally, regarding the beforeEach and afterEach hooks for the cover, these hooks are called twice when the cover and the homepage are on the same page. Since we cannot intuitively determine the target of the hook within the hook function, I have not implemented them.

@trusktr

I think we can merge this without having to fix the async stuff if that is not ready. It's a step forward, and we have to fix async at some point anyway

Choose a reason for hiding this comment

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

I believe this can go in without async marked support. It would still be an upgrade, and we can make sure to document in which cases async marked mode currently does not work.

@YiiGuxing would you mind adding docs for marked async mode to make this clear? Then we'll be a step closer.

I need to also review the code, but generally I was also annoyed that I couldn't plug into all markdown rendering of the site and only the main content.

Ideally a plugin should be able to handle any section of markdown (navbar, sidebar, cover, main, etc) so we should definitely go in this direction.

@YiiGuxing

@trusktr Theoretically all cases compiled directly by Compiler.compile without embed.prerenderEmbed preprocessing will not work in marked.js asynchronous mode, such as sidebar, navigation and cover (of course cover has added embed.prerenderEmbed preprocessing in this PR to support embedding external files). The reason for this problem is that our compiler does not provide support for marked.js asynchronous mode. In asynchronous mode, the product of marked.js during compilation would be a Promise that the compiler could not handle and an exception would occur.
I don't think this issue has anything to do with this PR, I just found it when I was implementing this PR, and I'm not sure where the docs you're talking about should be added appropriately. I think it would be more appropriate to open a new issue request to add support for marked.js asynchronous mode, which I will do if you agree.

Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.