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

update template to fix active page #1134

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karthik2804
Copy link
Contributor

Switching from highlighting active elements based on a partial match to needing a full match for /kubernetes as that is a substring in other URLs.

If we prefer to update everything to need a full match to keep up with consistency, I can do that as well. But I think I will make a follow-up PR cleaning up templates a little bit.

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
@karthik2804 karthik2804 marked this pull request as draft January 11, 2024 23:47
@karthik2804
Copy link
Contributor Author

Will update the PR to have wider clean ups and make this more consistent across all templates.

@@ -251,17 +251,21 @@
Kubernetes
</label>
<ul class="menu-list accordion-menu-item-content">
<li><a {{#if (active_project request.spin-full-url "/spin/v2/kubernetes" )}} class="active"
{{/if}} href="{{site.info.base_url}}/spin/v2/kubernetes">Spin on Kubernetes</a>
<li><a {{#if (active_content request.spin-path-info "/kubernetes" )}} class="active" {{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @karthik2804
My apologies; I don't understand the changes here active_content vs active_project and path-info vs full-url.
When I preview the documentation side by side with a checkout of this PR they seem to have the same behaviour. I feel like I am totally missing something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HI @tpmccallum This address the issue pointed out in this PR comment

@tpmccallum
Copy link
Contributor

Will be sure to check this is fully resolved as part of the v3_ToC work.
Looks like there are other PR associated with this; but will test to be sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants