-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
|
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 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?
why is the
navbeing moved? If there is an existingnavon the page, shouldn't it be left alone?
Because the mobile sidebar requires
There was a problem hiding this 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?
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 docsify/test/e2e/ that create a new test file (nav.test.js), check if it rendered as expect. :)
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); }); });
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>
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>
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?
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.
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!
There was a problem hiding this 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.
@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.
There was a problem hiding this comment.
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');
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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>
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.
There was a problem hiding this comment.
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.
@Evidlo --
Checking in. 😄
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
> docsify@4.12.2 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
> docsify@4.12.2 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
Seems like they are passing here: https://github.com/docsifyjs/docsify/actions
Did you try npm install?
@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).
There was a problem hiding this 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.
There was a problem hiding this comment.
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>.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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>
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.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
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: