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

Hide the navbar #872

Closed
oren opened this issue Jul 2, 2019 · 28 comments · Fixed by #1026
Closed

Hide the navbar #872

oren opened this issue Jul 2, 2019 · 28 comments · Fixed by #1026
Labels
bug confirmed as a bug

Comments

@oren
Copy link

oren commented Jul 2, 2019

Feature request

What problem does this feature solve?

Some sub sections of my website don't need navbar at all. I created a new project with the init command and all I have is this:

    window.$docsify = {
      name: '',
      repo: ''

but the navbar is showing. I don't need navbar for every section of my website.

What does the proposed API look like?

I don't know. This might be a bug or I just don't know how to use the existing API

How should this be implemented in your opinion?

Not sure

Are you willing to work on this yourself?

Depends on the complexity

@oren
Copy link
Author

oren commented Jul 4, 2019

I think currently the sidebar is always visible. I assume that based on this line:
https://github.com/docsifyjs/docsify/blob/develop/src/core/render/tpl.js#L49

@oren
Copy link
Author

oren commented Jul 4, 2019

I worked around it by hiding and showing the icon based on the location.hash. Dirty but works:

      window.addEventListener('hashchange', hashHandler, false);

      function hashHandler() {
        if(location.hash.startsWith("#/surf/")) {
          document.getElementsByClassName("sidebar-toggle-button")[0].style.display = "block";
        } else {
          document.getElementsByClassName("sidebar-toggle-button")[0].style.display = "none";
        }
      }

@anikethsaha
Copy link
Member

did you try this loadNavbar: false ?

@anikethsaha anikethsaha added the wait for information something is not clear, waiting for the author of the issue/pr label Nov 24, 2019
@methodbox
Copy link

did you try this loadNavbar: false ?

You know none of these flags actually work?

@anikethsaha
Copy link
Member

anikethsaha commented Dec 31, 2019

did you try this loadNavbar: false ?

You know none of these flags actually work?

I didnt get you
do you mean that passing false is not working ?

@anikethsaha
Copy link
Member

By default both sidebar and navbar are false which means, they are not visible.

@methodbox
Copy link

methodbox commented Dec 31, 2019

By default both sidebar and navbar are false which means, they are not visible.

Then why is there a side nav shown by default, and why doesn’t sideBar: “mysidebar.md”do anything?

Setting sidebar: “false” doesn’t hide it, either, and since it won’t load mine, it’s useless.

It looks like navBar: “mynavbar.md” works, but since I can’t get rid of (and it doesn’t do so automatically, which seems it should) the side bar, they overlap.

@anikethsaha
Copy link
Member

Thanks for the details 👍
I will try to reproduce it locally.

It seems like a bug here

!this.config.loadSidebar && this._renderSidebar()

@anikethsaha anikethsaha added bug confirmed as a bug and removed wait for information something is not clear, waiting for the author of the issue/pr labels Dec 31, 2019
@trusktr
Copy link
Member

trusktr commented Jan 1, 2020

Then why is there a side nav shown by default, and why doesn’t sideBar: “mysidebar.md”do anything?

There might be bugs that we should fix. We're in the process of adding unit tests, so hopefully any regressions won't happen in the future. We're not the original authors, so we're not 100% aware of any regressions, if any, but if you see anything, please do open an issue. That'd help a lot!

@trusktr
Copy link
Member

trusktr commented Jan 1, 2020

Here's an idea for a workaround: you could add a CSS style that applies display: none on certain pages for the navbar, and on other pages make sure to remove that CSS style.

Another idea: you could listen to route changes, and add/remove the CSS style on route changes.

@methodbox
Copy link

Yep, totally understand how I can work around it, but then, if I’m going to take the time to build custom logic or CSS for and “auto”-generated site, then it isn’t very auto, is it?

The goal of a tool like this seems like it should be to be fairly effortless and definitely shouldn’t require writing additional code, otherwise I could just use JSDoc out of the box and do the custom theme work myself.

@anikethsaha
Copy link
Member

We are sorry for this issue.
I will find a time soon to fix this one.

This project was not active for a long time and we are finding time in between to resolves these types of issues. we don't have tests to support us either. having tests would pick these kinds of bugs.
We think too that we should not have workarounds to get our stuff going but at this point, this is the quick fix.

If you find the fix then feel free to submit a PR to help others 👍

@methodbox
Copy link

We are sorry for this issue.
I will find a time soon to fix this one.

This project was not active for a long time and we are finding time in between to resolves these types of issues. we don't have tests to support us either. having tests would pick these kinds of bugs.
We think too that we should not have workarounds to get our stuff going but at this point, this is the quick fix.

If you find the fix then feel free to submit a PR to help others 👍

Will do. I might take a look tomorrow.

@anikethsaha
Copy link
Member

Will do. I might take a look tomorrow.

Cool

@anikethsaha
Copy link
Member

After looking at it.
loadNavBar options is working fine as expected. Please check whether you have <nav> tag in your HTML markup or not !!

loadSideBar and loadNavBar set to false

When they are set to false, they don't read for _sidebar.mdand_navBar.mdfile and don't render their content. **But**,sidebarstill gets render because of theheadingtags present in yourhomepage MD file (Default: README.md), both the config are doing their purpose of rendering _sidebar.mdand_navBar.md`.

Perhaps, if you want to hide the sidebar, we can introduce an options hideSideBar to fix it.

@anikethsaha
Copy link
Member

Here's an idea for a workaround: you could add a CSS style that applies display: none on certain pages for the navbar, and on other pages make sure to remove that CSS style.

We can totally do this and release a new plugin with the hideSidebar option provided by the plugin. But one thing about supporting it directly from the core is that it will be performance-friendly if we support this through core.

cause simply hiding the sidebar will fix the problem but docsify will still do all the steps of rendering, so if we are hiding it anyway, why wasting time in rendering it in first place. !!!!!

Should we go for plugin or support from the core?

cc @docsifyjs/core @trusktr @jthegedus @Koooooo-7

@anikethsaha
Copy link
Member

Check the implementation from the core.

anikethsaha@9b218a1#diff-250dfe7435d59fa33e80765c47190dd5R95

@Koooooo-7
Copy link
Member

@anikethsaha
tbh, Idk how many situations that we need to hide the sidebar(and hide the search plugins),
it seems like a static pages web. 😅 :
About the hideSidebar,I think it can be merged to loadSidebar = false.cuz I think current loadSidebar=false is useless.
Although I think this configuration(hide sidebar) is not so necessary now ,If we hv more configurations to satisfy different needs, it is better.

@anikethsaha
Copy link
Member

About the hideSidebar,I think it can be merged to loadSidebar = false.cuz I think current loadSidebar=false is useless.

by default, we use loadSidebar = false so docsify won't show sidebar by default if we change it to this.

I would have preferred the plugin cause this are user's preference choices and core should not be changed for every little enhancement but due to perf factors, i am leaning more towards adding it to the core.

@Koooooo-7
Copy link
Member

Koooooo-7 commented Feb 14, 2020

I would have preferred the plugin cause this are user's preference choices and core should not be changed for every little enhancement but due to perf factors

yep, u r right.
for ur implementation , I wish there would hv a better way instead of setting the styling back.:joy:

@anikethsaha
Copy link
Member

, I wish there would hv a better way instead of setting the styling back.😂

Yep,,,,I see that too...you have better option?

Also, I am going with implementing that in the core !!

@anikethsaha
Copy link
Member

@Koooooo-7 do you want to take this in a better way? else I will go with my implementation !!

or you want to continue as a follow-up of the anikethsaha/docsify@9b218a1 solution

@Koooooo-7
Copy link
Member

Koooooo-7 commented Feb 14, 2020

@anikethsaha I dont find a better way right now.:joy:
Although, I found that perhaps we can change the tpl.js.

  return (
    (isMobile ? `${aside}<main>` : `<main>${aside}`) +
    '<section class="content">' +
    '<article class="markdown-section" id="main"><!--main--></article>' +
    '</section>' +
    '</main>'
  )

I dont think it is a good way either...I haven't found when the stylings were setted yet.
I think u can make ur implementation at now, if someone finds a good way , he may update it then.:dog:

@anikethsaha
Copy link
Member

@anikethsaha I dont find a better way right now.😂
Although, I found that perhaps we can change the tpl.js.

  return (
    (isMobile ? `${aside}<main>` : `<main>${aside}`) +
    '<section class="content">' +
    '<article class="markdown-section" id="main"><!--main--></article>' +
    '</section>' +
    '</main>'
  )

I think this approach needs either new class name or again need to do the same styling here !!!

@Koooooo-7
Copy link
Member

@anikethsaha yep, I prefer to ur implementation.

@methodbox
Copy link

About the hideSidebar,I think it can be merged to loadSidebar = false.cuz I think current loadSidebar=false is useless.

by default, we use loadSidebar = false so docsify won't show sidebar by default if we change it to this.

I would have preferred the plugin cause this are user's preference choices and core should not be changed for every little enhancement but due to perf factors, i am leaning more towards adding it to the core.

I think this might be the part everyone is missing in this thread: loadSideBar set to false doesn’t actually work.

If the purpose of this flag is to show/not show the sidebar why would you create another key to hide the sidebar? And if it’s set to false to begin with, why does it still show anyway?

It seems like this should be a proper boolean which explicitly shows/hides the sidebar.

It should be as simple as conditionally rendering the component like:

function SideBar(props) {
  return (
    <>
      {props.showSideBar ?
        <div>
          ...sidebar content
        </div>
      : null}
    </>
  )
}

@Koooooo-7
Copy link
Member

Koooooo-7 commented Feb 15, 2020

I think this might be the part everyone is missing in this thread: loadSideBar set to false doesn’t actually work.

Currently, when loadSidebar = false,it would render that the left part are the search box and the titles(and subtitles) of content instead the sidebar.
Personally, I think it is useless.but if someone needs this, it would be useful.

If the purpose of this flag is to show/not show the sidebar why would you create another key to hide the sidebar? And if it’s set to false to begin with, why does it still show anyway?

When LoadSidebar = false, it will render as I said above.
When HideSidebar = true, it will render that the content only, no search box and no titles on the left.
Those configurations seems a little confused, maybe we need change to another names and distinguish them in another way.

@anikethsaha
Copy link
Member

as I said loadSidebar is not meant to hide/display the whole sidebar.
we need a separate option for that so we are going with hideSidebar for now !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed as a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants