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.

Sign up for GitHub

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

[embedding] feat: allow user configured navbar site #1535

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

[embedding] feat: allow user configured navbar site #1535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c12c68e
allow user configured navbar selection
Evidlo Mar 10, 2021
2b0db83
Merge branch 'develop' into develop
sy-records Mar 11, 2021
bb46a88
Merge branch 'develop' into develop
sy-records Mar 12, 2021
69def1c
Merge branch 'develop' into develop
sy-records Mar 28, 2021
a989914
Merge branch 'develop' into develop
Koooooo-7 Apr 12, 2021
f830976
Merge branch 'develop' into develop
sy-records Jun 23, 2021
fe30b70
add tests for nav_el option
Evidlo Jun 23, 2021
cf05a6f
fix tests to use innerHTML
Evidlo Jun 23, 2021
74a0af0
Merge branch 'develop' into develop
Evidlo Jul 31, 2021
8e5f080
Merge branch 'develop' into develop
trusktr Aug 2, 2021
d9871e1
Merge branch 'develop' into develop
Koooooo-7 Aug 5, 2021
738c39a
rename to camel case
Evidlo Jan 12, 2022
cdde500
dont merge nav when using custom nav element
Evidlo Jan 13, 2022
288d05d
Merge branch 'develop' into develop
Evidlo Jan 13, 2022
02daac4
use navEl when rendering
Evidlo Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,19 @@ window.$docsify = {
};
```

## navEl

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

Choose a reason for hiding this comment

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

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.


```js
window.$docsify = {
nav_el: '#navbar,
Copy link
Member

Choose a reason for hiding this comment

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

The configuration option would be navEl, not nav_el.

};
```

## loadSidebar

- Type: `Boolean|String`
Expand Down
14 changes: 7 additions & 7 deletions src/core/render/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,10 @@ export function Render(Base) {
}

_renderNav(text) {
text && this._renderTo('nav', this.compiler.compile(text));
text &&
this._renderTo(this.config.navEl || 'nav', this.compiler.compile(text));
if (this.config.loadNavbar) {
getAndActive(this.router, 'nav');
getAndActive(this.router, this.config.navEl || 'nav');
}
}

Expand Down Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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');

Copy link
Author

Choose a reason for hiding this comment

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

Won't this break a lot of existing configurations?

Copy link
Author

Choose a reason for hiding this comment

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

Also, should this be a CSS id rather than class since there is probably only one app-nav?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Of course the user could modify the wrapper so the nav has a matching selector.

Also not sure what should happen on mobile

Copy link
Member

@jhildenbiddle jhildenbiddle Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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 nav selectors 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 the index.html file 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>

CleanShot 2022-03-24 at 16 50 42@2x

CleanShot 2022-03-24 at 16 51 03@2x

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

const el = dom.find(config.el || '#app');
const navEl = dom.find(config.navEl || 'nav') || dom.create('nav');

const el = dom.find(id);
let html = '';
let navAppendToTarget = dom.body;

Expand Down Expand Up @@ -442,7 +442,7 @@ export function Render(Base) {
this.rendered = true;
}

if (config.mergeNavbar && isMobile) {
if (config.mergeNavbar && isMobile && !config.navEl) {
navAppendToTarget = dom.find('.sidebar');
} else {
navEl.classList.add('app-nav');
Expand All @@ -453,7 +453,7 @@ export function Render(Base) {
}

// Add nav
if (config.loadNavbar) {
if (config.loadNavbar && !config.navEl) {
dom.before(navAppendToTarget, navEl);
}

Expand Down
38 changes: 38 additions & 0 deletions test/e2e/nav.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
const docsifyInit = require('../helpers/docsify-init');

describe(`Navbar tests`, function() {
test('specify custom navbar element in config with nav_el', async () => {
const docsifyInitConfig = {
html: `
<html>
<body>
<nav id="mynav"></nav>
<div id="app"></nav>
Copy link
Member

Choose a reason for hiding this comment

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

You are closing <div id="app"> with </nav>. This should close with </div>.

</body>
</html>
`,
markdown: {
navbar: `
- [Foo](foo)
- [Bar](bar)
`,
homepage: `
# hello world
foo
`,
},
config: {
nav_el: '#mynav',
},
};

await docsifyInit(docsifyInitConfig);

// Check that our custom <nav> element contains nav links
let navHTML;
navHTML = await page.$eval('#mynav', el => el.innerHTML);
expect(navHTML).toMatch('<li><a href="#/foo" title="Foo">Foo</a></li>');
navHTML = await page.$eval('#mynav', el => el.innerHTML);
expect(navHTML).toMatch('<li><a href="#/bar" title="Bar">Bar</a></li>');
});
});