Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/D2Ve21HARenKP2wyfyRnL2KvxDCR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 02daac4:
|
Also, I was a bit confused about a few lines in On line 404 we have: which selects the first Later on line 446: // Add nav
if (config.loadNavbar) {
dom.before(navAppendToTarget, navEl);
}why is the |
Because the mobile sidebar requires |
Which file should this test go in? I've looked through the tests and I'm unsure. |
I think you could write the test under this folder |
I've been trying to write tests, but I'm really struggling to figure out Jest/Playwright When I specify
describe(`Navbar tests`, function() {
test('specify custom navbar element in config', async () => {
const docsifyInitConfig = {
html: `
<html>
<body>
<nav id="mynav"></nav>
</body>
</html>
`,
homepage: `# hello`
};
await docsifyInit(docsifyInitConfig);
});
});However, if I just comment out describe(`Navbar tests`, function() {
test('specify custom navbar element in config', async () => {
const docsifyInitConfig = {
// html: `
// <html>
// <body>
// <nav id="mynav"></nav>
// </body>
// </html>
// `,
homepage: `# hello`
};
await docsifyInit(docsifyInitConfig);
});
}); |
I think you're missing the try <!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
</head>
<body>
<div id="app"></div>
<nav id="mynav"></nav>
</body>
</html> |
Thanks that worked. However, I'm not seeing the describe(`Navbar tests`, function() {
test('specify custom navbar element in config', async () => {
const docsifyInitConfig = {
_logHTML: true,
html: `
<html>
<body>
<div id="app"></nav>
<nav id="mynav"></nav>
</body>
</html>
`,
markdown: {
navbar: `
- [Foo](foo)
- [Bar](bar)
`,
homepage: `
# hello world
foo
`,
},
config: {
nav_el: '#mynav',
},
};
await docsifyInit(docsifyInitConfig);
await expect(page).toHaveText(
'#mynav',
'some content will go here',
);
});
}); |
OK, this is a problem I ran into a few months ago when I initially implemented this: If I put the <html>
<body>
<nav id="mynav"></nav>
<div id="app"></nav>
</body>
</html>So maybe that should be considered a bug? |
OK, test is added and passing. Anything else that needs to happen before merge? |
Bump. The requested changes have been applied and the tests pass. |
Yeah, that is funky. Mind opening a bug report?
Awesome! Thanks for this! |
There was a problem hiding this comment.
Thanks @Evidlo for proposing this! I left some TODOs in the review comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
@jhildenbiddle tests keep flaking. Can you 👀 ? |
@trusktr Bump |
@Evidlo sorry we took long on this. Circling back. |
closes #1525 |
I believe everything is fixed now. |
| @@ -409,10 +410,9 @@ export function Render(Base) { | |||
| window.__current_docsify_compiler__ = this.compiler; | |||
| } | |||
| const id = config.el || '#app'; | |||
| const navEl = dom.find('nav') || dom.create('nav'); | |||
There was a problem hiding this comment.
We can solve this issue by using more specific CSS selectors instead of adding another configuration option:
- Desktop:
nav.app-nav - Mobile:
main > aside.sidebar > nav
const navEl = dom.find('nav.app-nav, main > aside.sidebar > nav') || dom.create('nav');There was a problem hiding this comment.
Won't this break a lot of existing configurations?
There was a problem hiding this comment.
Also, should this be a CSS id rather than class since there is probably only one app-nav?
There was a problem hiding this comment.
@jhildenbiddle that doesn't cover the use case that a person embeds docsify into a wrapper that has a nav on the outside, and that nav matching neither of those two selectors. That's what the main use case is here.
There was a problem hiding this comment.
Of course the user could modify the wrapper so the nav has a matching selector.
Also not sure what should happen on mobile
There was a problem hiding this comment.
@Evidlo --
The updated selector will not break existing configurations because it targets the same element: the nav element generated by Docsify.
Rather than write a lengthy comment explaining what changes need to be made, I made the necessary changes available in new fix-1525 branch. The changes are as follows:
- Updated all necessary
navselectors to the new selector listed above:nav.app-nav, main > aside.sidebar > nav - Fixed nav insert logic to ensure it is placed in the correct location in the DOM (adjacent to other docsify elements)
- Added a temporary custom
<nav id="mynav">element to theindex.htmlfile to demonstrate the changes
Feel free to check out the branch, build, and test to determine if these changes address your issue. FWIW, here are desktop and mobile screenshots of the demo with the custom <nav id="mynav"> element using the following markup:
<body>
<nav id="mynav" style=" background: red; color: white; text-align: center; padding: 1em; font-weight: bold;">
<p>Custom Nav Element</p>
</nav>
<div id="app">Loading ...</div>
</body>
I'll close by saying that the changes I've made here are intended to fix the issue without modifying the HTML output so that we can publish the fix as a non-breaking change. There are better ways to solve this problem and there are a lot of things about the UI rendering that we can and should clean up, but that is work for a later date IMHO.
There was a problem hiding this comment.
@trusktr --
that doesn't cover the use case that a person embeds docsify into a wrapper that has a nav on the outside, and that nav matching neither of those two selectors. That's what the main use case is here.
Perhaps we're interpreting the issue differently. Here's what @Evidlo said in #1525:
I'm having a problem where docsify is hijacking my template's
<nav>tag.
This is what the new selectors address: they prevent docsify from targeting the first <nav> element in the DOM by increasing the specificity. Based on the demo link provided, I'm assuming the goal is to prevent docsify from overwriting the existing top navigation (i.e., "hijacking" the orange banner with link to Home, Guidelines, Lab, etc.)
@Evidlo -- Let me know if I misinterpreted here.
@Evidlo -- Checking in. 😄 |
We'll it seems the tests on |
Seems like they are passing here: https://.com/docsifyjs/docsify/actions Did you try |
@Evidlo --
Your markup is incorrect. You're closing |
| <html> | ||
| <body> | ||
| <nav id="mynav"></nav> | ||
| <div id="app"></nav> |
There was a problem hiding this comment.
You are closing <div id="app"> with </nav>. This should close with </div>.
| ```js | ||
| window.$docsify = { | ||
| nav_el: '#navbar, |
There was a problem hiding this comment.
The configuration option would be navEl, not nav_el.
| - Type: `String` | ||
| - Default: `null` | ||
| The DOM element to use for rendering the navbar. It can be a CSS selector string or an actual [HTMLElement](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement). If `null`, the first `<nav>` element on the page is used. If it doesn't exist, it is created at the top of the DOM. |
There was a problem hiding this comment.
Based on the source, this option accepts a string only (not an HTMLElement). Either the description or the source needs to be updated to match.
| @@ -409,10 +410,9 @@ export function Render(Base) { | |||
| window.__current_docsify_compiler__ = this.compiler; | |||
| } | |||
| const id = config.el || '#app'; | |||
| const navEl = dom.find('nav') || dom.create('nav'); | |||
There was a problem hiding this comment.
@Evidlo --
The updated selector will not break existing configurations because it targets the same element: the nav element generated by Docsify.
Rather than write a lengthy comment explaining what changes need to be made, I made the necessary changes available in new fix-1525 branch. The changes are as follows:
- Updated all necessary
navselectors to the new selector listed above:nav.app-nav, main > aside.sidebar > nav - Fixed nav insert logic to ensure it is placed in the correct location in the DOM (adjacent to other docsify elements)
- Added a temporary custom
<nav id="mynav">element to theindex.htmlfile to demonstrate the changes
Feel free to check out the branch, build, and test to determine if these changes address your issue. FWIW, here are desktop and mobile screenshots of the demo with the custom <nav id="mynav"> element using the following markup:
<body>
<nav id="mynav" style=" background: red; color: white; text-align: center; padding: 1em; font-weight: bold;">
<p>Custom Nav Element</p>
</nav>
<div id="app">Loading ...</div>
</body>
I'll close by saying that the changes I've made here are intended to fix the issue without modifying the HTML output so that we can publish the fix as a non-breaking change. There are better ways to solve this problem and there are a lot of things about the UI rendering that we can and should clean up, but that is work for a later date IMHO.
| @@ -409,10 +410,9 @@ export function Render(Base) { | |||
| window.__current_docsify_compiler__ = this.compiler; | |||
| } | |||
| const id = config.el || '#app'; | |||
| const navEl = dom.find('nav') || dom.create('nav'); | |||
There was a problem hiding this comment.
@trusktr --
that doesn't cover the use case that a person embeds docsify into a wrapper that has a nav on the outside, and that nav matching neither of those two selectors. That's what the main use case is here.
Perhaps we're interpreting the issue differently. Here's what @Evidlo said in #1525:
I'm having a problem where docsify is hijacking my template's
<nav>tag.
This is what the new selectors address: they prevent docsify from targeting the first <nav> element in the DOM by increasing the specificity. Based on the demo link provided, I'm assuming the goal is to prevent docsify from overwriting the existing top navigation (i.e., "hijacking" the orange banner with link to Home, Guidelines, Lab, etc.)
@Evidlo -- Let me know if I misinterpreted here.
Summary
render/index.jsselects the first<nav>element on the page to use as docsify's navbar. This makes embedding on existing pages problematic when the page already includes a<nav>. This change adds anav_elconfig option that allows selecting which navbar to use on the page, in the same manner as theeloption.There are a few key behaviors implemented:
nav_elis not found, fall back to selecting the first<nav>tagnav_elis defined, do not try to append to the top of the DOMView a demo here.
What kind of change does this PR introduce?
Feature
For any code change,
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
Related issue, if any:
closes #1525
Tested in the following browsers: