GTD-3 Multipage Navigation #27
Conversation
…d by table_of_contents and if you pass through content that's been through a layout you end up with infinite recursion.
…ge url before the fragment id
…rk for single pages
…them to the bottom of the navigation instead of crashing.
…or heading structure, as it increments the heading ids, and causes a mismatch between the navigation and the page content
…rls that include paths as well as fragment ids
… match for the current browser location. This results in less false positives
…l within them (for single page docs)
Add dedicated <button> elements to toggle the tree Update aria attributes and button text on toggle
Also add the url to the error message to help locate the pages to fix
Heading class no longer requires page_url but headings builder still does, all headings should have an associated url
There was a problem hiding this comment.
Looking good!
Two general comments:
- Is there a good way in which we can test this? It would be good to have an integration test for some of the behaviour
- I wonder if this needs to be optional behaviour. It works a bit oddly with the current setup of the GOV.UK docs. If it was a configuration option we could merge this sooner and test it without compatibility issues.
I haven't reviewed the JS/CSS.
| @@ -110,6 +110,17 @@ title: My beautiful page | |||
| --- | |||
| ``` | |||
|
|
|||
| ## `weight` | |||
|
|
|||
| Affects the order a page is displayed in the sidebar navigation tree. Lower | |||
There was a problem hiding this comment.
This confused me a bit because a higher weight in search results often means that things will end up higher in the results. What would you think of flipping the logic and naming this navigation_priority?
Big 👍 for documenting the option.
There was a problem hiding this comment.
Sure, that seems more explicit. I'll ask @jonathanglassman for his thoughts.
There was a problem hiding this comment.
I'm happy with either. "Weight = heavier sinks to the bottom" made sense to me, but navigation priority also makes sense. I would teach it as "smaller numbers = higher up the TOC" so as long as people don't think that higher numbers mean a higher priority in the TOC, we should be ok. If I had to make a choice I would keep it as is.
There was a problem hiding this comment.
Yeah I see your point, it would be nice to have something that just makes sense without an explanation. It's hard to come up with a scale that's intuitive.
Jon and I had a quick chat this morning, and decided to stick with weight for now and he really connected with the analogy. However, we want to push this out wider to other writers and see if they get confused by this.
There was a problem hiding this comment.
I haven't had time to look at the PR as a whole yet, but just wanted to "weigh in" on this discussion...
I would interpret weight in the same way as @tijmenb - as in: more weight = more important = more prominent.
I would maybe call it something like order or navigation_order.
There was a problem hiding this comment.
(navigation_)priority would be good. (navigation_)order may be vague 🤔
lib/source/layouts/layout.erb
Outdated
| max_level: config[:tech_docs][:max_toc_heading_level] | ||
| ) | ||
|
|
||
| # Only use html files |
There was a problem hiding this comment.
Perhaps this can be extracted into a helper, so we can test it separately.
Also tidied up the doc a bit now that it's being linted
I had to restructure the function a little to pass in Middleman globals
Checks for links to other page headings on the home page navigation
|
Ok, I've written some tests, I copied a lot from the meta tags unit tests.
I was thinking we could release this on a major version, then some sites can choose to hold back until it's ready. |
|
@lewisnyman I would like to release as few breaking versions of this as possible. Existing sites won't always have a developer available so upgrading should be made as easy as possible. Also I'm not sure this particular pattern will work for GOV.UK, so I'd like to keep the option of using the existing functionality. |
|
I have a few questions that aren't really related to the PR, but are more about the feature in general. Happy to discuss offline if this isn't the best place for it.
I think it's fine if we say this is an optional feature that not all sites will use, but I'm wary about putting too much choice in the hands of the whoever's consuming the gem by making things like this opt-in. We could be more opinionated about how it should be used by the various projects as long as we have a simple migration path. There are certain kinds of documentation (like API docs) that could follow a very standard approach and we already have a lot of guidance about the kinds of things we expect to be there. If someone's already thought about the general information architecture for that use case, then I would prefer for the gem to encourage that approach for users who are spinning up new sites. Users of the gem should be able to set something up quickly without doing a lot of design work, and we should avoid solving similar problems differently across different teams. |
|
@MatMoore responses to your questions:
Agree that we should provide guidance on how it should be used. The standard approaches that we use for certain types of content like API docs are still compatible with this approach. |
|
Here's a visual update: |
|
Screenshots on iPhone 5s and iPhone X |
Setting this to true will add headings from other pages to the sidebar navigation This does not currently disable the collapsible javascript
|
I've added a boolean option: This option does no affect the collapsible navigation UI, at least not yet. It feels like there might be some value in having this for single page sites. |
|
We ran a testing workshop on the new multipage functionality. Here's some of the feedback related to the comment above: We had some feedback that collapsible navigation would be useful on sites that wanted to stick with a single page structure eg Registers. It sounds like it's a good idea to keep that functionality separate to the the multipage_nav option. Does the collapsible functionality needs it's own option to enable? I haven't seen a situation where that is the case yet. |
In preparation for adding support for child pages in multipage navigation
|
We also had a discussion on the weight issue. Although it seemed to make intuitive sense, there are potential complications arising from implementing multipage functionality with groupings and "nested" subpages. @lewisnyman will build this subpage functionality and then we can test if the weight method works, or if we need to implement another method, such as a TOC page that recreates the page hierarchy in a single markdown file. |
…llapsible navigation All paths contain the path of the root page: /
Navigation items of pages that have sub-pages do not print fragments in their href
… and cofiguration option The javascript file is still included, but it is not used
There is now a separate option to enable the collapsible sidebar navigation. This means that someone can benefit from the collapsible top level nav items without splitting their content into multiple pages. Registers is an example of a site that would benefit from this. |
Pages with children don't print the fragment id in href
Fixes alphagov/tech-docs-template#128.
This adds multiple page navigation support for the sidebar navigation. When you create additional .html.md files, they'll be included in the sidebar, along with the headings on that page. These are collapsed by default.
I've got this running against a fork of the paas content here: ConvivioTeam/paas-tech-docs/tree/GTD-14-prototype-multipage-navigation
Currently it only displays the top level pages, it doesn't' support nested pages within pages. I'm eager to get this used as soon as possible so we can figure out where we need more flexibility. This is my first PR against this project so nitpicking is welcome!