-
Notifications
You must be signed in to change notification settings - Fork 256
[SPIKE] Improve how navigation data is computed #4669
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
base: spike-flat-mobile-nav
Are you sure you want to change the base?
[SPIKE] Improve how navigation data is computed #4669
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify project configuration. |
{%- set navLinks = [] %} | ||
{%- for item in navigation %} | ||
{%- set navLinks = (navLinks.push({ | ||
href: '/' + item.url + '/', | ||
text: item.label, | ||
current: true if permalink and (permalink == item.url) else false, | ||
active: true if permalink and permalink.startsWith(item.url) else false | ||
href: item.href, | ||
text: item.text, | ||
current: item.isCurrent(permalink), | ||
active: item.isActive(permalink) | ||
}), navLinks) %} | ||
{%- endfor %} |
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.
Would love for this to become something like a call to navigation.topLevel(permalink)
that'd output data directly consumable by the Service Navigation rather than relying on the weird set
inside the loop.
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.
Given we'll be keeping the accordion, we'll need that loop anyways so it's not really an issue that we need to call item.isCurrent
or item.isActive
.
14ed7db
to
0cc70f6
Compare
a7a209e
to
74d268d
Compare
This aligns with Service Navigation semantics and allows us to make future changes without changing the templates
…arch Avoids new concepts of 'label' and 'url' where we already have 'title' and 'permalink'. If nuance needs to be introduced, specific plugins can introduce that closer to the code that use these nuances
This makes all navigation items be sorted on the same property for consistency. To identify initial sections, we can rely on the `showSubNav` property, though we could use a custom `includeInNavigation` property which would trigger both the page being listed in the top-level navigation and the rendering of the sub navigation.
Simplifies the code inside the loop
Ensures both navigation and search represent sections the same way
Use built in JavaScript function and ensures the grouping happens only once per build rather than once (if not twice when rendering the section overviews) per page render
0cc70f6
to
cb908af
Compare
Explores steps we could take to improve how we generate the data used to render the various navigation menus (main navigation after the header, sidebar navigation, navigation duplicated in section's index page) as well as sitemap.
Before diving in, you may want to read about how the different parts of the current codebase contribute to the navigation (internal link).
The general direction would be to:
text
andhref
) rather than custom concepts oflabel
orurl
, particularly havinghref
a value you can set directly as anhref
attribute on a link rather than have to remember to prefix and suffix it with/
The spike introduces:
A. Computing the list of items in the top level navigation from the files provided by Metalsmith rather than a hardcoded list within
config/navigation.js
, one less file to maintainB. A
NavigationItem
class to derive navigation data from Metalsmith's file item, deriving thetext
from the page'stitle
and thehref
from the page's permalink. It also provides methods for checking if a permalink represents the "current" or "active" page, which will allow this to be tested in the future.C. Using the concept of 'section' to compute sub-navigation items so that both the navigation and search have the same concept for what constitute a section of the site
D. Grouping of the sub-navigation items by theme in our navigation plugin rather than in each template. This ensures both consistent grouping across pages, but importantly means the grouping is only computed once per build.
Thoughts
Overall, this feels like a step towards a cleaner structure for the navigation data and could allow us to have that part of the site better tested.
We may want to steer away from using getters for the
NavigationItem
(or a class altogether even, if we prefer), as Nunjucks'groupBy
helper seemed to struggle with them before the grouping by theme was moved to the JavaScript.Explore moving the computation of the sub navigation to JavaScript
Both the
subnav
andcategory-nav
templates do the same process of finding the active item in the navigation to render the list of sub-pages.This could be moved to a
getSubNavigation
JavaScript method, which could be nicely tested and simplify the template's loop to:Explore formalising the parent-child relations between pages
It feels like there's a big overlap between the navigation itself (top level and sub-navigation), the sitemap, and the search in terms of how the tree of pages is reprensented. As we're also introducing breadcrumbs, which also need to navigate that tree (albeit from leaves to root, rather than from root to leaf as we do currently), I'm wondering if having proper
parent
andchildren
properties may help extract data more easily.Reorder when plugins run in Metalsmith.
In the same spirit of separating data computations from rendering, it'd be great if all data got computed before any rendering happened. This would allow the sitemap to be generated in a page rather than a layout.
Update content structure to surface the top level navigation
Also thinking it could be nice to structure the content in the following way to highlight which pages are part of the navigation and lift them up at the start of the content:
We can easily drop the
0X_
part of the path, but theexample
macro seems to stumble as it cannot find the files to render. Seems a piece of work for a future spike.