feat: The timing of the doneEach hook call for the cover
#2427
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 |
|
@jhildenbiddle. Completed. Please review. |
|
@jhildenbiddle, I found that this PR does not work in the asynchronous mode of |
|
@jhildenbiddle, The fact that the cover doesn't work in asynchronous mode on Additionally, regarding the |
|
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 |
trusktr
left a comment
trusktr
left a comment
There was a problem hiding this comment.
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 |
| // However, when the cover and the home page are on the same page, | ||
| // the 'beforeEach' and 'afterEach' hooks are called multiple times. | ||
| // It is difficult to determine the target of the hook within the | ||
| // hook functions. We might need to make some changes. |
There was a problem hiding this comment.
@YiiGuxing I think we should update the hooks in a backwards compatible way, such that the hook can somehow see which content it is called for. For example it can see string values like "cover", "sidebar", etc. The default (right now no sort of value to determine the target of the hook) would work the same as before for anyone who hasn't updated their plugin code.
There was a problem hiding this comment.
The ultimate result should be, that all markdown files are handled the same, regardless where in the app they are rendered (cover, sidebar, nav, etc) but any plugin can easily decide which to handle.
Something like
hook.doneEach((section) => {
if (section === 'cover') {
// run logic after cover rendered
} else if (section === 'sidebar') {
// run logic after sidebar rendered
} else if (section === 'nav') {
// run logic after nav bar rendered
} else if (section === 'main') {
// run logic after main content rendered (new way)
} else {
// run logic after main content rendered (old way, fallback)
}
})or similar.
|
@trusktr is attempting to deploy a commit to the Docsify Team on Vercel. A member of the Team first needs to authorize it. |
|
@YiiGuxing sorry for taking so long. This is a really change. Updated with |
| _renderCover(text, coverOnly, next) { | ||
| const el = dom.getNode('.cover'); | ||
| const rootElm = document.documentElement; | ||
| const coverBg = getComputedStyle(rootElm).getPropertyValue('--cover-bg'); |
There was a problem hiding this comment.
This variable is now unused in the PR. Is the feature relating to that now deleted? If so, we need to fix that.
Also there are type errors I'm fixing (my recent changes added type checking to the code base).
There was a problem hiding this comment.
Now that tests run, a jest test is failing. https://github.com/docsifyjs/docsify/actions/runs/19981379473/job/57308351783?pr=2427
There was a problem hiding this comment.
I fixed the error, a method moved to the utils file.
Now the only thing failing is snapshots for the cover page. The diff shows this failure:
Looks like a couple classes are missing.
There was a problem hiding this comment.
Here's the visual difference:
Before:
After (broken):
Maybe I missed something when merging in develop?
| } | ||
| // Gradient background | ||
| else { | ||
| const degrees = Math.round((Math.random() * 120) / 2); |
There was a problem hiding this comment.
Looks like this gradient background code is removed. Need to restore it. @YiiGuxing would you mind restoring the random gradient?
5 participants
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: