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

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

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

Conversation

Evidlo
Copy link

@Evidlo Evidlo commented Mar 10, 2021

Summary

render/index.js selects 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 a nav_el config option that allows selecting which navbar to use on the page, in the same manner as the el option.

There are a few key behaviors implemented:

  • if nav_el is not found, fall back to selecting the first <nav> tag
  • if nav_el is defined, do not try to append to the top of the DOM

View a demo here.

What kind of change does this PR introduce?

Feature

For any code change,

  • Related documentation has been updated if needed
  • Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

closes #1525

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercel bot commented Mar 10, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/D2Ve21HARenKP2wyfyRnL2KvxDCR
✅ Preview: https://docsify-preview-git-fork-evidlo-develop-docsify-core.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2021

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:

Sandbox Source
docsify-template Configuration

@Evidlo
Copy link
Author

Evidlo commented Mar 10, 2021

Also, I was a bit confused about a few lines in index.js:

On line 404 we have:

const navEl = dom.find('nav') || dom.create('nav');

which selects the first nav, or creates a new element if necessary. Nothing wrong there.

Later on line 446:

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

why is the nav being moved? If there is an existing nav on the page, shouldn't it be left alone?

@sy-records
Copy link
Member

why is the nav being moved? If there is an existing nav on the page, shouldn't it be left alone?

Because the mobile sidebar requires

@sy-records sy-records changed the title allow user configured navbar selection feat: allow user configured navbar selection Mar 11, 2021
Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

Can you add tests to it?

@Evidlo
Copy link
Author

Evidlo commented Mar 13, 2021

Which file should this test go in? I've looked through the tests and I'm unsure.

@Koooooo-7
Copy link
Member

I think you could write the test under this folder docsify/test/e2e/ that create a new test file (nav.test.js), check if it rendered as expect. :)

@Evidlo
Copy link
Author

Evidlo commented Jun 23, 2021

I've been trying to write tests, but I'm really struggling to figure out Jest/Playwright

When I specify html in the docsifyInitConfig options, the tests timeout:

test/e2e/nav.test.js

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);
  });
});
 FAIL   browser: firefox e2e  test/e2e/nav.test.js (16.944 s)
  Navbar tests
    ✕ specify custom navbar element in config (15471 ms)

  ● Navbar tests › specify custom navbar element in config

    on@http://127.0.0.1:3001/lib/docsify.js:203:1

      scrollActiveSidebar@http://127.0.0.1:3001/lib/docsify.js:2402:7
      renderMixin/proto._bindEventOnRendered@http://127.0.0.1:3001/lib/docsify.js:8204:26
      initFetch@http://127.0.0.1:3001/lib/docsify.js:9014:10
      initMixin/proto._init@http://127.0.0.1:3001/lib/docsify.js:9034:16
      Docsify@http://127.0.0.1:3001/lib/docsify.js:9088:10
      @http://127.0.0.1:3001/lib/docsify.js:9108:39
      setTimeout handler*documentReady@http://127.0.0.1:3001/lib/docsify.js:242:14
      @http://127.0.0.1:3001/lib/docsify.js:9108:16
      @http://127.0.0.1:3001/lib/docsify.js:9110:2

  ● Navbar tests › specify custom navbar element in config

    thrown: "Exceeded timeout of 15000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      41 | // });
      42 | describe(`Navbar tests`, function() {
    > 43 |   test('specify custom navbar element in config', async () => {
         |   ^
      44 |     const docsifyInitConfig = {
      45 |       html: `
      46 |         <html>

      at test/e2e/nav.test.js:43:3
      at Object.<anonymous> (test/e2e/nav.test.js:42:1)

However, if I just comment out html in the config, the test runs fine:

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

@sy-records
Copy link
Member

I think you're missing the <div id="app"></div>

try

      <!DOCTYPE html>
      <html>
        <head>
          <meta charset="UTF-8" />
        </head>
        <body>
          <div id="app"></div>
          <nav id="mynav"></nav>
        </body>
      </html>

@Evidlo
Copy link
Author

Evidlo commented Jun 23, 2021

Thanks that worked. However, I'm not seeing the <nav> element appear anywhere in the output HTML:

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',
    );
  });
});
<html>
  <head>
    <script src="/lib/docsify.js"></script>
    <title></title>
  </head>
  <body data-page="README.md" class="ready sticky">
    <main>
      <button class="sidebar-toggle" aria-label="Menu">
        <div class="sidebar-toggle-button">
          <span></span><span></span><span></span>
        </div>
      </button>
      <aside class="sidebar">
        <div class="sidebar-nav">
          <ul>
            <li>
              <a
                class="section-link"
                href="#/?id=hello-world"
                title="hello world"
                >hello world</a
              >
            </li>
          </ul>
        </div>
      </aside>
      <section class="content">
        <article class="markdown-section" id="main">
          <h1 id="hello-world">
            <a href="#/?id=hello-world" data-id="hello-world" class="anchor"
              ><span>hello world</span></a
            >
          </h1>
          <p>foo</p>
        </article>
      </section>
    </main>
    <div class="progress" style="opacity: 0; width: 0%;"></div>
  </body>
</html>

@Evidlo
Copy link
Author

Evidlo commented Jun 23, 2021

OK, this is a problem I ran into a few months ago when I initially implemented this:

If I put the <nav> element after <div id="app"></div>, then it seem to get overwritten. However, putting it before works:

        <html>
            <body>
            <nav id="mynav"></nav>
            <div id="app"></nav>
            </body>
        </html>

So maybe that should be considered a bug?

@Evidlo
Copy link
Author

Evidlo commented Jun 25, 2021

OK, test is added and passing. Anything else that needs to happen before merge?

@Evidlo
Copy link
Author

Evidlo commented Jul 31, 2021

Bump. The requested changes have been applied and the tests pass.

@trusktr
Copy link
Member

trusktr commented Aug 2, 2021

OK, this is a problem I ran into a few months ago when I initially implemented this:

If I put the <nav> element after <div id="app"></div>, then it seem to get overwritten. However, putting it before works:

        <html>
            <body>
            <nav id="mynav"></nav>
            <div id="app"></nav>
            </body>
        </html>

So maybe that should be considered a bug?

Yeah, that is funky. Mind opening a bug report?

Bump. The requested changes have been applied and the tests pass.

Awesome! Thanks for this!

@trusktr trusktr added the semver-minor This needs a semver-minor release label Aug 2, 2021
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.

Thanks @Evidlo for proposing this! I left some TODOs in the review comments.

src/core/render/index.js Outdated Show resolved Hide resolved
src/core/render/index.js Outdated Show resolved Hide resolved
src/core/render/index.js Outdated Show resolved Hide resolved
src/core/render/index.js Outdated Show resolved Hide resolved
src/core/render/index.js Outdated Show resolved Hide resolved
@trusktr
Copy link
Member

trusktr commented Aug 2, 2021

@jhildenbiddle tests keep flaking. Can you 👀 ?

@Evidlo
Copy link
Author

Evidlo commented Aug 20, 2021

@trusktr Bump

@trusktr
Copy link
Member

trusktr commented Jan 4, 2022

@Evidlo sorry we took long on this. Circling back.

@trusktr trusktr changed the title feat: allow user configured navbar selection [embedding] feat: allow user configured navbar site Jan 5, 2022
@trusktr
Copy link
Member

trusktr commented Jan 12, 2022

closes #1525

@Evidlo
Copy link
Author

Evidlo commented Jan 14, 2022

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

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.

@jhildenbiddle
Copy link
Member

@Evidlo --

Checking in. 😄

@Evidlo
Copy link
Author

Evidlo commented Mar 23, 2022

We'll it seems the tests on develop are failing now:

[evan@blackbox docsify] git status
On branch origindevelop
Your branch is up to date with 'origin/develop'.

nothing to commit, working tree clean
[evan@blackbox docsify] npm run build:js

> [email protected] build:js
> cross-env NODE_ENV=production node build/build.js

lib/plugins/ga.js
lib/plugins/ga.min.js
lib/plugins/matomo.js
lib/plugins/matomo.min.js
lib/plugins/external-script.js
lib/plugins/external-script.min.js
lib/plugins/disqus.js
lib/plugins/disqus.min.js
lib/plugins/gitalk.js
lib/plugins/gitalk.min.js
lib/plugins/emoji.js
lib/plugins/emoji.min.js
lib/plugins/zoom-image.js
lib/plugins/zoom-image.min.js
lib/plugins/search.js
lib/plugins/search.min.js
lib/plugins/front-matter.js
lib/plugins/front-matter.min.js
lib/docsify.js
lib/docsify.min.js

[evan@blackbox docsify] npm test

> [email protected] test
> jest && run-s test:e2e

Determining test suites to run...

[Browsersync] Access URLs:
 -------------------------------------
    Local: http://localhost:3001
 External: http://192.168.207.109:3001
 -------------------------------------
[Browsersync] Serving files from: /home/evan/resources/docsify/test


 FAIL   unit  test/unit/example.test.js
  ● Example Tests › Fake Timers › data & time

    TypeError: setSystemTime is not available when not using modern timers

      59 | 
      60 |       jest.useFakeTimers();
    > 61 |       jest.setSystemTime(fakeDate);
         |            ^
      62 | 
      63 |       const timeOfDay = getTimeOfDay();
      64 | 

      at Object.setSystemTime (node_modules/jest-runtime/build/index.js:1777:17)
      at Object.<anonymous> (test/unit/example.test.js:61:12)

 PASS   integration  test/integration/example.test.js
 PASS   integration  test/integration/emoji.test.js
 PASS   integration  test/integration/docs.test.js
 PASS   integration  test/integration/docsify.test.js
 PASS   unit  test/unit/render-util.test.js
 PASS   unit  test/unit/core-util.test.js
 PASS   integration  test/integration/global-apis.test.js
 PASS   unit  test/unit/router-util.test.js
 PASS   unit  test/unit/router-history-base.test.js
 PASS   integration  test/integration/render.test.js (5.488 s)

Test Suites: 1 failed, 10 passed, 11 total
Tests:       1 failed, 67 passed, 68 total
Snapshots:   36 passed, 36 total
Time:        7.403 s
Ran all test suites in 2 projects.
npm ERR! code 1
npm ERR! path /home/evan/resources/docsify
npm ERR! command failed
npm ERR! command sh -c jest && run-s test:e2e

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/evan/.npm/_logs/2022-03-23T04_50_17_203Z-debug.log

@trusktr
Copy link
Member

trusktr commented Mar 23, 2022

Seems like they are passing here: https://github.com/docsifyjs/docsify/actions

Did you try npm install?

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Mar 23, 2022

@Evidlo --

If I put the <nav> element after <div id="app"></div>, then it seem to get overwritten. However, putting it before works:

       <html>
           <body>
           <nav id="mynav"></nav>
           <div id="app"></nav>
           </body>
       </html>

So maybe that should be considered a bug?

Your markup is incorrect. You're closing <div id="app"> with </nav>. If you update your markup to close <div id="app"> correctly, I believe you will not experience this issue (haven't tested though).

Copy link
Member

@jhildenbiddle jhildenbiddle left a comment

Choose a reason for hiding this comment

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

See fix-1525 branch for proposed changes and testing.

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


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

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

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

@jhildenbiddle jhildenbiddle Mar 24, 2022

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.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor This needs a semver-minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to specify the nav element
5 participants