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

Allow headers to point to index pages in Nav. #1042

Closed
wants to merge 3 commits into from
Closed

Conversation

@waylan
Copy link
Member

@waylan waylan commented Aug 29, 2016

If the first child is a Nav Header is a index.md page, then that page
is assigned as the URL of the Header. The problem is that you can't
actually access it from the Nav as the click is interrupted to display/hide
the dropdown list of children.

Almost fixes #73.

Also need to update the readthedocs theme.

@kamar535
Copy link

@kamar535 kamar535 commented Nov 1, 2016

I'm not sure I fully understand this but does this solve the issue I reported here https://groups.google.com/forum/#!topic/mkdocs/OAGGtYrA7Xg

@waylan
Copy link
Member Author

@waylan waylan commented Nov 1, 2016

@kamar535 yes this is directly related. If this was working, then your problem wouldn't exist. What I have added so far in this PR is the background data to allow you to build what you want in the template. Unfortunately, the problem is that the current user interface (in the included themes) for the nav doesn't make sense with this change. In other words, before this change can be applied we also need to make some changes to the themes. However, I'm not sure that will ever happen. See this comment for the reason why we may elect to not support this; in which case you would only ever be able to have an index page at the root, but not in any sub-directories.

@waylan waylan force-pushed the waylan:73 branch from a09b863 to 550d0e6 Nov 1, 2016
@waylan
Copy link
Member Author

@waylan waylan commented Nov 1, 2016

I just pushed a fix for the MkDocs theme nav bar. The trick was to combine Bootstrap's Split Button Dropdowns with the Navbar. Here's a screenshot of the relevant part of the navbar:

screencapture-127-0-0-1-8000-about-1478030796053

Note the faint line between the "About" link (to the /about/ page) and the dropdown arrow. It works, but it could be easy for users to miss that there are two seperate links there.

Still need to address the readthedocs theme, which may be tricky as (according to the Bootstrap docs) split button dropdowns are not supported on vertical button groups.

@waylan
Copy link
Member Author

@waylan waylan commented Nov 1, 2016

So apparently the readthedocs theme is based on this Sphinx theme, which is built on top of the Wyrm (SASS) framework. This is completely different than Bootstrap.

@waylan
Copy link
Member Author

@waylan waylan commented Nov 2, 2016

Sigh. In Sphinx the nav is built by the template calling the toctree callable, which builds the nav as a docutils document object and then renders that as a document fragment which is written to the template. In other words, the nav is built programically in Python code. Although there is a code comment which does provide some insight into the intended structure. For reference, here is a tidied up fragment directly from the source of the ReadTheDocs documentation:

<p class="caption"><span class="caption-text">User Documentation</span></p>

<ul class="current">
    <li class="toctree-l1 current">
        <a class="current reference internal" href="">Getting Started</a>
        <ul>
            <li class="toctree-l2">
                <a class="reference internal" href="#write-your-docs">Write Your Docs</a>
                <ul>
                    <li class="toctree-l3">
                        <a class="reference internal" href="#in-restructuredtext">In reStructuredText</a>
                    </li>
                    <li class="toctree-l3">
                        <a class="reference internal" href="#in-markdown">In Markdown</a>
                    </li>
                </ul>
            </li>
            <li class="toctree-l2">
                <a class="reference internal" href="#import-your-docs">Import Your Docs</a>
            </li>
        </ul>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="versions.html">Versions</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="builds.html">Build Process</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="features.html">Read the Docs features</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="support.html">Support</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="faq.html">Frequently Asked Questions</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="yaml-config.html">Read the Docs YAML Config</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="guides/index.html">Guides</a>
    </li>
</ul>

<p class="caption"><span class="caption-text">About Read the Docs</span></p>

<ul>
    <li class="toctree-l1">
        <a class="reference internal" href="contribute.html">Contributing to Read the Docs</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="team.html">Read the Docs Team</a></li>
    <li class="toctree-l1">
        <a class="reference internal" href="ethical-advertising.html">Ethical Advertising</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="sponsors.html">Sponsors of Read the Docs</a>
    </li>
    <li class="toctree-l1">
        <a class="reference internal" href="open-source-philosophy.html">Read the Docs Open Source Philosophy</a>
    </li>
</ul>
waylan added 2 commits Aug 29, 2016
If the first child in a Nav Header is a `index.md` page, then that page
is assigned as the URL of the Header.

Mkdocs theme updated to use Bootstrap's split button dropdown for Nav
Headers with URLs.

Almost fixes #73. Also need to update the readthedocs theme.
* Fix a nested `<li>` bug.
* Add support for section headers with links (fixes #73).
* More closely replicate nesting of the Sphinx theme nav (related to #588).
@waylan waylan force-pushed the waylan:73 branch from 550d0e6 to 68f0cb0 Nov 2, 2016
@waylan waylan added Update needed and removed Update needed labels Nov 2, 2016
@waylan
Copy link
Member Author

@waylan waylan commented Nov 2, 2016

Okay, I think this is mostly done. The documentation still needs updated.

I had to refactor the readthedocs theme navigation (which had some bugs I discovered) and now it actually more closely mirrors the Sphinx theme it is a clone of. I think any further improvements would come under #588. I should point out that there is a weirdness where Sphinx only supports section heading and one sub-level of navigation, whereas MkDocs supports an arbitrary level. This causes a slight inconsistency which is hard to work around. Perhaps we should document that if you are using the readthedocs theme, to use the more restrictive document structure.

@waylan waylan removed the Update needed label Nov 2, 2016
@kamar535
Copy link

@kamar535 kamar535 commented Nov 15, 2016

Nice! I can't wait to see this get merged into master.

@waylan
Copy link
Member Author

@waylan waylan commented Nov 15, 2016

The current patch adds a note to the 0.16.0 release notes. That release has already happened to this needs to be updated. I'll do that when we determine what the next (not point) release will be (0.17 or 1.0). Then we can probably merge it.

@kamar535
Copy link

@kamar535 kamar535 commented Nov 15, 2016

@waylan I'm quite new to Git, Github and pull requests. How can I test you patch before the next release?

@waylan
Copy link
Member Author

@waylan waylan commented Nov 15, 2016

@kamar535 you can download and install the branch which this PR is coming from. The easiest way would be to do the following (preferably in a virtual env):

pip install https://github.com/waylan/mkdocs/archive/73.zip

And please feel free to provide any feedback. I should also note that the bug reported in #1089 exists in this PR, but the bug also exists outside of it, so this doesn't introduce the bug. Just noting that I am aware that it exists.

@kamar535
Copy link

@kamar535 kamar535 commented Nov 16, 2016

I've just managed to use virtualenv to install your patched version of mkdocs.

First I tried with the following:

site_name: My Docs

theme: readthedocs

pages:
  - home: index.md
  - sub:  sub/index.md
      - next: sub/next.md

, but then I got the following error:

ScannerError: mapping values are not allowed here

Next I fond this #73 (comment) and changed to:

site_name: My Docs

theme: readthedocs

pages:
  - home: index.md
  - simple:
      - about: about.md
      - contact: contact.md
  - sub: 
      - sub/index.md
      - next: sub/next.md
  - foo:
      - foo/index.md
      - bar: foo/bar.md

, and now works almost like I want it to.

I have the following file structure:

.
├── docs
│   ├── about.md
│   ├── contact.md
│   ├── foo
│   │   ├── bar.md
│   │   └── index.md
│   ├── index.md
│   └── sub
│       ├── index.md
│       └── next.md
└── mkdocs.yml

After clicking on the simple > about navitem:

screen shot 2016-11-16 at 01 04 44

Note how the header About page is indented under the about page navitem and that it is clear that contact refers to a separate page.

After clicking on the sub nav item:

screen shot 2016-11-16 at 01 02 10

, the navitem next is shown at the same level and with the same background color as the headings on the sub/index.md page although it is a separate page.

At the moment I'm not sure how to make it clear that next is not a header on the sub/index.md page but a separate page. Any suggestions?

@waylan
Copy link
Member Author

@waylan waylan commented Nov 16, 2016

@kamar535 you found the problem with adding this feature (and why this has the "needs design decision" label). The readthedocs theme does not work well with this structure. In fact, Sphinx does not even allow index pages in subdirectories. As our readthedocs theme is a clone if a Sphinx theme, then it will never support this feature properly. I see three ways to resolve this:

  1. Follow Sphinx's lead and remove all support for index pages in subdirectories.
  2. Apply the existing patch (perhaps with a few tweaks) and document that it is not supported by the readthedocs theme.
  3. Leave things as-is and document that index pages are not supported in subdirectories.

Option 1 would break many existing users sites, so that seems unlikely. Personally, I'm inclined to do option 3. That way existing sites (with index pages) will still work (as they do today), but new sites won't use index pages (if they read our documentation) and the issue will be mostly eliminated. After all, the much more popular Sphinx project has been quite successful without offering the option.

waylan referenced this pull request in d0ugal/mkdocs Dec 3, 2016
This is very much a work in progress - but it is starting to become
functional.

Outstanding issues:

- The grey for the menu/header should extend to be the fully width of
  the screen
- Footer is missing
- Search is missing
- Need to check all the other features etc.
- Cross browser testing
- The menu doesn't look great when it overlaps with white
- Some things like headings and the logo are hacked in for now, need to
  think about how we handle this properly
- What do we call this theme? do we replace the mkdocs theme? if we do?
  where does that go? move it to the other bootswatch themes and give it
  back the original bootswatch name (Cerulean iirc)?
- probably lots of other stuff...
@waylan
Copy link
Member Author

@waylan waylan commented Dec 9, 2016

Just stumbled upon another potential problem with this. I completely ignored the "mobile" view. Maybe its fine, but I never even checked it. This just occurred to me while reviewing #1107.

@gdaow
Copy link

@gdaow gdaow commented Jan 20, 2017

My two cents : because you check if an index.md page is the first child to add a link on the nav header, it will likely not work with auto-populated pages (moreover, that feature makes auto-populating an even more valuable approach). Really cool feature anyway, I hope a solution can be found for the Readthedocs theme, that'd allow this to be merged.

@waylan
Copy link
Member Author

@waylan waylan commented Jan 20, 2017

Yes, this will never work with an auto-populated pages config. Another reason to not include this feature.

@gdaow
Copy link

@gdaow gdaow commented Jan 20, 2017

For this particular case, I guess that looking for and index.md file anywhere in the children collection would work ?

@squidfunk
Copy link

@squidfunk squidfunk commented Jan 21, 2017

This would be a cool and much awaited feature. However, my suggestion: don't do too much magic. I think this could have a lot of problems with some existing themes, as it completely alters the behavior of the button/menu. I think it would be better to provide some option to automatically link section headings to the first child page. This could possibly be done with a simple true/false flag inside mkdocs.yml or directly inside the template. I'm thinking about including it in https://github.com/squidfunk/mkdocs-material.

@omenos
Copy link

@omenos omenos commented Mar 31, 2017

This could possibly be done with a simple true/false flag inside mkdocs.yml or directly inside the template. I'm thinking about including it in https://github.com/squidfunk/mkdocs-material.

This would be the most ideal situation in my opinion. Maintain what is currently the standard, but allow users the ability to extend just a bit farther for the smaller use cases. The Material theme is what I use and the built in arrow/dropdown structure for navigation makes this feel more intuitive and obvious than some of the built in themes.

@waylan
Copy link
Member Author

@waylan waylan commented Mar 31, 2017

Another concern with the implementation attached to this PR is that it is dependent on the file structure. An interesting use case was presented in #1187 where the desire is to have a nav structure which does not mirror the file/directory structure.

I'm open to using something other than the directory structure to define the page for a header, but its not clear what that would look like.

The common report from users is to try something like this:

Parent: path/to/parent.md
    - Child: path/to/child.md

But that is not valid YAML. As a reminder, YAML is a subset of JSON. How would you represent that as JSON? You can't (go ahead, try to alter the next example to make it work). Of course, this is easy:

Parent: 
    - Child 1: path/to/child1.md
    - Child 2: path/to/child2.md

becomes

{ "Parent":  
    [
        { "Child 1": "path/to/child1.md" },
        { "Child 2": "path/to/child2.md" }
    ]
}

So how do we represent the "parent" URL and differentiate it from any other page?

Some things that won't work are:

  1. Use the child item with a missing a title. This doesn't fit with auto-generated pages as none of them include a title. Besides, In 1.0 we will have support for defining the title in the page's frontmatter/meta data (which is already implemented in the 1.0.dev branch) so a user could conceivably have a title defined for every page; just not in the config file.
  2. Use the first child item. Again, this won't work well for autogenerated pages.
  3. Use the index.md file. This requires that the nav structure mirrors the file structure.
  4. Use a "special title" in the config (if the title of a page matches [some pattern], then it represents the path for the parent header). This feels a little to "magic" to me. Also, which title do you use if one is defined in the frontmatter?

We could add some config setting which would alter the behavior. However, as has been pointed out, this is very much dependent on the theme having support. The fact is, a theme could easily implement any of the above methods within a template (by altering the way it loops through the nav items) with no changes made to MkDocs at all. And as part of #1164 themes will get their own settings. So a theme could conceivably include a setting to enable/disable such a feature. The real question here is whether it is reasonable to expect theme authors to do this within a template. If not, then I don't want MkDocs to alter its behavior based on a theme specific setting (and we should include a setting separate from the theme). But if so, then that seems like a better solution to me.

@captainsubtle
Copy link

@captainsubtle captainsubtle commented Sep 15, 2017

👍 this would be great to have, any update or should I just use the fork?

@runtman
Copy link

@runtman runtman commented Sep 15, 2017

+1 ^ <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.