-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
feat: The timing of the doneEach hook call for the cover
#2427
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
Conversation
The `doneEach` hook function for the cover should be called after the cover page HTML has been appended to the DOM.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thanks of this, @YiiGuxing. Can you add some tests to test/e2e/plugins.test.js to verify the new behavior? Thx!
@jhildenbiddle. Completed. Please review.
@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.
@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.
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
There was a problem hiding this 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.
@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.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The
doneEachhook 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,
Does this PR introduce a breaking change?
No
Tested in the following browsers: