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

docs: adapt built in acetate helper to fix sticky links #334

Closed
wants to merge 1 commit into from

Conversation

jgravois
Copy link
Contributor

ISSUES CLOSED: #284

🎩 tip to @paulcpederson for pointing out that its actually a helper built right into acetate that helps us set the is-active class automatically when a page loads.

it feels like a wretched hack to me to copy/paste the entire helper (and its dependencies) into this library, but i couldn't think of a more clever way to extend the logic in the comparisons.

${BASE_URL}${currentUrl}` === `finalUrl`

// instead of 
currentUrl === finalUrl

@paulcpederson also mentioned that we might should be using {{ relativePath }} in this website to make our lives easier, but i couldn't get that option to behave either.

{% link relativePath + "api/", "API Reference", class="side-nav-link"%}

// .api/ ?

{% link relativePath + "/api/", "API Reference", class="side-nav-link"%}

// /api/ ??

@coveralls
Copy link

coveralls commented Sep 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling eafcdd1 on docs/284 into be11aa1 on master.

@paulcpederson
Copy link
Member

@jgravois you could also just not use the link helper rather than replicating it and changing it inside your project.

This seems to work pretty well:

<nav class="top-nav-list" role="navigation" aria-labelledby="topnav">
  <a href="{{relativePath}}/guides/" class="{% if url == '/guides/'%}is-active{% endif %} top-nav-link">Guides</a>
  <a href="{{relativePath}}/api/" class="{% if url == '/api/'%}is-active{% endif %} top-nav-link">API Reference</a>
</nav>

@@ -211,4 +236,84 @@ module.exports = function(acetate) {
: [];
return `npm install ${package.name} ${peers.join(" ")}`;
});

acetate.helper(
"link",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jgravois this should already be built into Acetate. http://acetate.io/documentation/built-in-helpers/ > Link Helper. Is something wrong with that version? The follwoing should just work in templates {% link '/guide/', 'Guide' %}

@jgravois
Copy link
Contributor Author

jgravois commented Sep 21, 2018

thanks @paulcpederson. doing that for each of the individual 'link`s in the API reference sidebar probably is a better option than hacking on the built in helper.

@patrickarlt i wouldn't say there's anything wrong with what is built into acetate. it just doesn't account for the fact that we added a utility in #83 to prepend our baseurls with /arcgis-rest-js/ only when we are building for production.

{% link baseUrl + package.pageUrl, package.pkg.name %}

this was the best idea we could come up with to host the doc at https://esri.github.io/arcgis-rest-js/.

there's likely a way to configure the raw source to achieve both lofty goals simultaneously, but the exact path eludes me.

@jgravois
Copy link
Contributor Author

there's likely a way to configure the raw source to achieve both lofty goals simultaneously

figured it out in #341

@jgravois jgravois closed this Sep 27, 2018
@jgravois jgravois deleted the docs/284 branch October 5, 2018 19:41
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.

4 participants