-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat: add loadNavbarOnCover configuration #1376
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/jbpg8v8eg |
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 1f9e427:
|
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.
One change, other than that, LGTM
defc915
to
882c99f
Compare
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.
Little problem.
Steps:
- set
loadNavbarOnCover: false,
. - clink to another selection.
- clink the
back
button of the browser, it will back to the cover page.
the nav will show on the cover now.
This is caused by not reloading the page, I think that's acceptable. |
It make no sense, there is no reason to make an element visible, which had been set // Load nav
if (loadNavbar) {
// default show the nav
document.querySelector('.app-nav').style.display = 'inline';
if (path === '/' && !loadNavbarOnCover) {
// hidden nav on cover
document.querySelector('.app-nav').style.display = 'none';
return;
} Maybe you could get another brilliant one :). |
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.
Well.
I think the test is not a test actually, it just check the configuration property exist, not checking if it works tho.
Besides that, I think we need concern about refactoring the nav, for some reason. #1230
fix Update docs.index.html fix fix fix Update docs.index.html fix Update docs.index.html fix remove test Update docs.index.html update
Summary
resolve #1375
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.