docsify [embedding] feat: allow user configured navbar site

[embedding] feat: allow user configured navbar site

Open Evidlo opened this issue 3 years ago • 22 comments

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,

[x] 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
[x] No

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

Related issue, if any:

closes https://github.com/docsifyjs/docsify/issues/1525

Tested in the following browsers:

[x] Chrome
[x] Firefox
[ ] Safari
[ ] Edge
[ ] IE

Mar 10 '21 23:03 Evidlo

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

Mar 10 '21 23:03 vercel[bot]

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 02daac441462d7f0a5d574ee7c4e9d5a8b89b072:

Sandbox	Source
docsify-template	Configuration

Mar 10 '21 23:03 codesandbox-ci[bot]

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?

Mar 10 '21 23:03 Evidlo

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

Mar 11 '21 02:03 sy-records

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

Mar 13 '21 02:03 Evidlo

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

Mar 17 '21 13:03 Koooooo-7

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

Jun 23 '21 04:06 Evidlo

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>

Jun 23 '21 05:06 sy-records

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>

Jun 23 '21 05:06 Evidlo

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?

Jun 23 '21 05:06 Evidlo

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

Jun 25 '21 02:06 Evidlo

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

Jul 31 '21 20:07 Evidlo

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!

Aug 02 '21 00:08 trusktr

@jhildenbiddle tests keep flaking. Can you :eyes: ?

Aug 02 '21 01:08 trusktr

@trusktr Bump

Aug 20 '21 05:08 Evidlo

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

Jan 04 '22 07:01 trusktr

closes https://github.com/docsifyjs/docsify/issues/1525

Jan 12 '22 05:01 trusktr

I believe everything is fixed now.

Jan 14 '22 08:01 Evidlo

@Evidlo --

Checking in. :smile:

Mar 20 '22 14:03 jhildenbiddle

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

Mar 23 '22 04:03 Evidlo

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

Did you try npm install?

Mar 23 '22 16:03 trusktr

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

Mar 23 '22 18:03 jhildenbiddle

docsify docsify copied to clipboard

[embedding] feat: allow user configured navbar site

Summary

What kind of change does this PR introduce?

For any code change,

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

Related issue, if any:

Tested in the following browsers:

docsify
docsify copied to clipboard