Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Evidlo wants to merge15 commits intodocsifyjs:develop
base:develop
Choose a base branch
Loading
fromEvidlo:develop

Conversation

@Evidlo
Copy link

@EvidloEvidlo commentedMar 10, 2021•
edited by trusktr
Loading

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 anav_el config option that allows selecting which navbar to use on the page, in the same manner as theel option.

There are a few key behaviors implemented:

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

View a demohere.

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

vercelbot commentedMar 10, 2021•
edited
Loading

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-cibot commentedMar 10, 2021•
edited
Loading

This pull request is automatically built and testable inCodeSandbox.

To see build info of the built libraries, clickhere or the icon next to each commit SHA.

Latest deployment of this branch, based on commit02daac4:

SandboxSource
docsify-templateConfiguration

@Evidlo
Copy link
Author

Evidlo commentedMar 10, 2021•
edited
Loading

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

On line 404 we have:

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

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

Later on line 446:

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

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

@sy-records
Copy link
Member

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

Because the mobile sidebar requires

@sy-recordssy-records changed the titleallow user configured navbar selectionfeat: allow user configured navbar selectionMar 11, 2021
Copy link
Member

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

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 folderdocsify/test/e2e/ that create a new test file (nav.test.js), check if it rendered as expect. :)

@Evidlo
Copy link
Author

Evidlo commentedJun 23, 2021•
edited
Loading

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

When I specifyhtml in thedocsifyInitConfig options, the tests timeout:

test/e2e/nav.test.js

describe(`Navbar tests`,function(){test('specify custom navbar element in config',async()=>{constdocsifyInitConfig={html:`        <html>            <body>            <nav></nav>            </body>        </html>      `,homepage:`# hello`};awaitdocsifyInit(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 outhtml in the config, the test runs fine:

describe(`Navbar tests`,function(){test('specify custom navbar element in config',async()=>{constdocsifyInitConfig={// html: `//   <html>//       <body>//       <nav></nav>//       </body>//   </html>// `,homepage:`# hello`};awaitdocsifyInit(docsifyInitConfig);});});
Koooooo-7 reacted with thumbs up emoji

@sy-records
Copy link
Member

I think you're missing the<div></div>

try

<!DOCTYPE html><html><head><metacharset="UTF-8"/></head><body><divid="app"></div><navid="mynav"></nav></body></html>
Koooooo-7 reacted with thumbs up emoji

@Evidlo
Copy link
Author

Evidlo commentedJun 23, 2021•
edited
Loading

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()=>{constdocsifyInitConfig={_logHTML:true,html:`        <html>            <body>            <div></nav>            <nav></nav>            </body>        </html>      `,markdown:{navbar:`          - [Foo](foo)          - [Bar](bar)        `,homepage:`          # hello world          foo        `,},config:{nav_el:'#mynav',},};awaitdocsifyInit(docsifyInitConfig);awaitexpect(page).toHaveText('#mynav','some content will go here',);});});
<html>  <head>    <script src="/lib/docsify.js"></script>    <title></title>  </head>  <body data-page="README.md">    <main>      <button aria-label="Menu">        <div>          <span></span><span></span><span></span>        </div>      </button>      <aside>        <div>          <ul>            <li>              <a                               href="#/?id=hello-world"                title="hello world"                >hello world</a              >            </li>          </ul>        </div>      </aside>      <section>        <article>          <h1>            <a href="#/?id=hello-world" data-id="hello-world"              ><span>hello world</span></a            >          </h1>          <p>foo</p>        </article>      </section>    </main>    <div></div>  </body></html>

@Evidlo
Copy link
Author

Evidlo commentedJun 23, 2021•
edited
Loading

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></div>, then it seem to get overwritten. However, putting it before works:

<html><body><navid="mynav"></nav><divid="app"></nav></body></html>

So maybe that should be considered a bug?

@Evidlo
Copy link
Author

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

@Evidlo
Copy link
Author

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

@trusktr
Copy link
Member

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></div>, then it seem to get overwritten. However, putting it before works:

        <html>            <body>            <nav></nav>            <div></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!

@trusktrtrusktr added the semver-minorThis needs a semver-minor release labelAug 2, 2021
Copy link
Member

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

@trusktr
Copy link
Member

@jhildenbiddle tests keep flaking. Can you 👀 ?

@Evidlo
Copy link
Author

@trusktr Bump

@trusktr
Copy link
Member

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

@trusktrtrusktr changed the titlefeat: allow user configured navbar selection[embedding] feat: allow user configured navbar siteJan 5, 2022
@trusktr
Copy link
Member

closes#1525

@Evidlo
Copy link
Author

I believe everything is fixed now.

}

constid=config.el||'#app';
constnavEl=dom.find('nav')||dom.create('nav');

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
constnavEl=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 oneapp-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

@jhildenbiddlejhildenbiddleMar 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: thenav element generated by Docsify.

Rather than write a lengthy comment explaining what changes need to be made, I made the necessary changes available in newfix-1525 branch. The changes are as follows:

  • Updated all necessarynav 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> element to theindex.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> element using the following markup:

<body><navid="mynav"style=" background: red; color: white; text-align: center; padding: 1em; font-weight: bold;"><p>Custom Nav Element</p></nav><divid="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.

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 thedemo 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 commentedMar 23, 2022•
edited
Loading

We'll it seems the tests ondevelop are failing now:

[evan@blackbox docsify] git statusOn branch origindevelopYour branch is up to date with 'origin/develop'.nothing to commit, working tree clean[evan@blackbox docsify] npm run build:js> docsify@4.12.2 build:js> cross-env NODE_ENV=production node build/build.jslib/plugins/ga.jslib/plugins/ga.min.jslib/plugins/matomo.jslib/plugins/matomo.min.jslib/plugins/external-script.jslib/plugins/external-script.min.jslib/plugins/disqus.jslib/plugins/disqus.min.jslib/plugins/gitalk.jslib/plugins/gitalk.min.jslib/plugins/emoji.jslib/plugins/emoji.min.jslib/plugins/zoom-image.jslib/plugins/zoom-image.min.jslib/plugins/search.jslib/plugins/search.min.jslib/plugins/front-matter.jslib/plugins/front-matter.min.jslib/docsify.jslib/docsify.min.js[evan@blackbox docsify] npm test> docsify@4.12.2 test> jest && run-s test:e2eDetermining 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 totalTests:       1 failed, 67 passed, 68 totalSnapshots:   36 passed, 36 totalTime:        7.403 sRan all test suites in 2 projects.npm ERR! code 1npm ERR! path /home/evan/resources/docsifynpm ERR! command failednpm ERR! command sh -c jest && run-s test:e2enpm 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

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

Did you trynpm install?

@jhildenbiddle
Copy link
Member

jhildenbiddle commentedMar 23, 2022•
edited
Loading

@Evidlo --

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

<html><body><navid="mynav"></nav><divid="app"></nav></body></html>

So maybe that should be considered a bug?

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

Copy link
Member

@jhildenbiddlejhildenbiddle left a comment

Choose a reason for hiding this comment

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

Seefix-1525 branch for proposed changes and testing.

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

Choose a reason for hiding this comment

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

You are closing<div> with</nav>. This should close with</div>.


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

Choose a reason for hiding this comment

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

The configuration option would benavEl, notnav_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.

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.

}

constid=config.el||'#app';
constnavEl=dom.find('nav')||dom.create('nav');
Copy link
Member

@jhildenbiddlejhildenbiddleMar 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: thenav element generated by Docsify.

Rather than write a lengthy comment explaining what changes need to be made, I made the necessary changes available in newfix-1525 branch. The changes are as follows:

  • Updated all necessarynav 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> element to theindex.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> element using the following markup:

<body><navid="mynav"style=" background: red; color: white; text-align: center; padding: 1em; font-weight: bold;"><p>Custom Nav Element</p></nav><divid="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.

}

constid=config.el||'#app';
constnavEl=dom.find('nav')||dom.create('nav');

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 thedemo 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jhildenbiddlejhildenbiddlejhildenbiddle requested changes

@trusktrtrusktrAwaiting requested review from trusktr

@sy-recordssy-recordsAwaiting requested review from sy-records

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

semver-minorThis needs a semver-minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ability to specify thenav element

5 participants

@Evidlo@sy-records@Koooooo-7@trusktr@jhildenbiddle
[8]ページ先頭
©2009-2025 Movatter.jp