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

Adding dynamic navigation based on site.json #409

Merged
merged 2 commits into from
Apr 12, 2016
Merged

Adding dynamic navigation based on site.json #409

merged 2 commits into from
Apr 12, 2016

Conversation

mrkiffie
Copy link
Contributor

@mrkiffie mrkiffie commented Dec 5, 2015

Implements a solution for the menus, partially as described in #281.
Changes to site.json are to replicate the current state of the various sidebar menus

Menu Links

  • Are objects that have both a link and a text property
  • The link can be an internal path relative to the local. e.g docs/faq
  • The link can be relative to the root. e.g /api/
  • The link can be an external url. e.g https://github.com/nodejs/nodejs.org

Main menu

  • Uses an inline loop instead of a template as it is only used in once.
  • Consists of menu links found on the first level.
  • Displays menu links in the order in which they appear in site.json.
  • I removed trademark from the object to prevent it showing up in the main name

Sub menus

  • All menu link that appear in the main menu are also traversed to extract sub menus.
  • Sub menus include a reference to themselves as the first link (to replicate the current sidebar menu behaviour)
  • Example usage {{> navigation key='about'}}

Navigation Helper
The navigation helper adds the menus to the metadata under the property nav

@mrkiffie
Copy link
Contributor Author

mrkiffie commented Dec 6, 2015

@sup I've updated the PR - added a short description as you suggested. I also made a couple of formatting changes in the navigation helper that were failing on travis but not picked up pre-commit.

@mikeal
Copy link
Contributor

mikeal commented Dec 6, 2015

can we get a screenshot :)

@mrkiffie
Copy link
Contributor Author

mrkiffie commented Dec 7, 2015

Hey @mikeal, this doesn't change the appearance at all. It only changes way the navigation is managed.

Instead of having to maintain different menu templates for different sidebar menus, they share a generic template and the menus are defined in each locale's site.json.

The benefit will be more noticeable for additional translations of the site as they won't require the menu structure to be identical to the english version. (I'm not sure if this behaviour is desirable or not?)

@mikeal
Copy link
Contributor

mikeal commented Dec 7, 2015

ahhhh, ok :) 👍

@mrkiffie
Copy link
Contributor Author

The merge conflict above is a result of deleting layouts/partials/docs-menu.hbs which has since had additions (#324). The merge conflict can be easily resolved, but real conflict in this PR is in the direction it takes the sidebar menu.

@mrkiffie
Copy link
Contributor Author

mrkiffie commented Apr 8, 2016

I have resolved the merge conflict.

As far as my testing goes, I think that I've been able to replicate the current navigation.

I have added additional logic in the template to support the API doc's "LTS" label and the text is an optional subtext property in the locale's site.json.

Let me know if there are any additional considerations I have missed.

@fhemberger
Copy link
Contributor

@nodejs/website Can you please take a look and test it? This would be a huge improvement.

"api": {
"text": "API"
"api-lts": {
"link": "dist/latest-v4.x/docs/api",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok?
I think that dist/latest-v4.x/docs/api becomes /en/dist/latest-v4.x/docs/api/ after generation.
However true url is https://nodejs.org/dist/latest-v4.x/docs/api/.

dist/latest-v5.x/docs/api is also the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh hmm, no that isn't ok. The docs are not translated to begin with. (due to various other issues)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @abouthiroppy. I accidentally introduced this while trying to figure out how to handle the "LTS" label on the version 4 docs. I'll push a patch shortly.

@Fishrock123
Copy link
Contributor

Seems to work fine. LGTM

@mrkiffie
Copy link
Contributor Author

I've update the PR with the following changes:

  • Fix urls to LTS and Stable API docs - made relative to the domain's root instead of the locale
  • Fix malformed html by adding missing closing tags (whoops 😳)
  • Moving subtext conditional - because API docs start with a /

Please let me know if you spot anything else out of the ordinary.

@hiroppy
Copy link
Member

hiroppy commented Apr 12, 2016

LGTM 👍

@fhemberger fhemberger merged commit b5ca415 into nodejs:master Apr 12, 2016
@fhemberger
Copy link
Contributor

Merged, thank you all!

@mrkiffie mrkiffie deleted the dynamic-nav branch January 10, 2018 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants