search

tektoncd / website

Tekton Website
https://tekton.dev
Apache License 2.0
63 stars 151 forks source link

Update vault left nav #503

Closed geriom closed 1 year ago

geriom commented 1 year ago

Changes

This new layout is essentially the default docsy code with small changes to keep the menu scope in the correct subfolder.

/closes #465

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

See the contribution guide for more details.

AlanGreene commented 1 year ago

I only see this on pipelines and triggers docs. What do we need to do to get it to show for the other projects?

geriom commented 1 year ago

I only see this on pipelines and triggers docs. What do we need to do to get it to show for the other projects?

It seems we are reaching some sort of limit on the number of nav items we can go through. The site gives up at CLI v0.10.0 after checking every single file under /Pipelines and /Triggers. I might be running into some sort of cache issue. I'll keep looking for an answer.

tekton-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/tektoncd/website/blob/main/OWNERS)~~ [afrittoli] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
geriom commented 1 year ago

I only see this on pipelines and triggers docs. What do we need to do to get it to show for the other projects?

Fixed. The problem was that the macro generating the tree is a recursive definition, and we have so many elements in the vault we had a Stack Overflow, which silently just skipped the rest of the items. I adjusted the tree root to each subfolder in the vault. Prevents the overflow, simplifies the code and it's way more efficient too.

I renamed the LTS folders to use a dash because docsy really doesn't like spaces in folder names.

@AlanGreene this is ready for review.

AlanGreene commented 1 year ago

Unfortunately this seems to break the version dropdown. If I select a previous version the header still states 'Latest Versions' and there's no way to actually get back to the latest versions without navigating away and back into the docs root.

See the URL shows pipelines-v0.40.x and the correct nav + doc content, but the header and dropdown does not match and think they're showing latest. image

geriom commented 1 year ago

Unfortunately this seems to break the version dropdown. If I select a previous version the header still states 'Latest Versions' and there's no way to actually get back to the latest versions without navigating away and back into the docs root.

@AlanGreene Thank you for the thorough testing. I missed the switcher partial, I just added it back. I'm going to summarize, for posterity, what I'm doing here:

When the site was created we had two layouts, one for docs/ and one for vault/. These layouts were identical except for the nav tree and the version swticher. For every site update, both layouts had to be updated, which is easier said than done, and they started drifting apart; that's what broke the nav for vault/. Currently we don't need separate layouts, docsy supports declaring the type of doc in a folder, using the cascading: -type option, and the doc picks the layout that matches the type.

This PR deletes the redundant code (6 layout files), which will prevent drift and make it easier to maintain. Instead I'm using the default docs layout with flags that set the correct nav-tree and version-switcher menu depending on the current Path. The only significant difference is that the "Documentation" nav link on the top bar is always visible, which gives a quick way to go back to the main docs from the vault.

Let me know if you see anything else not working as expected.

AlanGreene commented 1 year ago

Great work @geriom, thanks for tackling this. All looks good now, and it even gets rid of the unwanted Vault entry in the sidebar that I hadn't noticed before. It's also nice having less code to maintain and fewer manual steps required on future upgrades. 👍

/lgtm /meow

tekton-robot commented 1 year ago

@AlanGreene: cat image

In response to [this](https://github.com/tektoncd/website/pull/503#issuecomment-1424467794): >Great work @geriom, thanks for tackling this. All looks good now, and it even gets rid of the unwanted Vault entry in the sidebar that I hadn't noticed before. It's also nice having less code to maintain and fewer manual steps required on future upgrades. 👍 > >/lgtm >/meow Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.