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

feat: The timing of the doneEach hook call for the cover #2427

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

Conversation

YiiGuxing
Copy link

@YiiGuxing YiiGuxing commented May 13, 2024

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.
Copy link

vercel bot commented May 13, 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 Jun 5, 2024 2:17pm

@YiiGuxing YiiGuxing marked this pull request as ready for review May 13, 2024 15:33
@jhildenbiddle
Copy link
Member

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

@YiiGuxing
Copy link
Author

@jhildenbiddle. Completed. Please review.

@YiiGuxing
Copy link
Author

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

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

@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
Copy link
Member

trusktr commented Feb 28, 2025

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

Copy link
Member

@trusktr trusktr left a comment

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
Copy link
Author

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

5 participants