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

Add links to jump to latest version of docs #1918

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pdavies
Copy link

@pdavies pdavies commented Jun 10, 2024

Hello! 👋

This PR addresses #1917 . It adds support for specifying which version is the latest via docs_config.js:

var versionNodes = [
    { version: 'v1.0.0', url: '/foo'},
    { version: 'v2.0.0', url: "/bar", latest: true },
    { version: 'v0.34.0-dev', url: "/baz" }
];

which will trigger a warning hyperlink to the latest version when viewing other versions:

image

It is not required to have a latest version in docs_config.js. Existing docs_config.js files will continue to produce the existing behaviour with no warning links.

Rather than try to determine which version is the latest through javascript logic, it seemed more appropriate to delegate to the author of docs_config.js, for example hexdocs. This avoids the risk of behaviour that differs from the documentation host's opinion on which version is the latest. This does mean that the PR doesn't do anything interesting without a small enhancement to (in practice) hexdocs.

This PR has the drawback that docs published prior to this PR will not get warnings. But due to the way exdocs works, I don't see how any attempt at fixing #1917 could get around this.

@josevalim
Copy link
Member

This looks good to me but the indicator is calling too much attention. What if we keep only the warning sign? And then on mouse over it says this is not the latest version and you can click it to update?

@halostatue
Copy link

@josevalim I think that it would be better to have the words than the warning sign for mobile users, since there is no "mouse over" capability there. I think the colours could definitely be muted, but from an accessibility point of view, words are better.

@josevalim
Copy link
Member

On mobile, we usually render the project name and version on every page, below the title, because the sidebar is not open. Shall we have the go to latest there instead (for mobile)?

@halostatue
Copy link

I think so, but it might be something worth rethinking the render, because having that as a parenthetical next to a parenthetical is odd.

mobile-ish rendering of Oban docs

@pdavies
Copy link
Author

pdavies commented Jun 12, 2024

Thanks for the feedback!

Visuals: I agree the white on red is unpleasant and should be toned down. It also did not play nicely with light/dark theming. However there is a balance to strike - if it isn't eyecatching, it isn't useful. I've revised the appearance to be theme-aware and less painful on the eyes. It still isn't beautiful but I'd need to defer to someone with better frontend design skills to improve it any further!

Mobile view: I hadn't given any thought to this. My preference would be to tackle that separately - more honestly for someone else to tackle that separately. This is mainly because I'm not sure how I would get the necessary information to module_template.eex in a nice way.

image image

@@ -9,5 +9,9 @@
</option>
{{/each}}
</select>
{{#if jumpToLatestUrl }}
<br />
<span class="sidebar-projectVersionsLatestLink"><a href="{{jumpToLatestUrl}}">Go to latest</a></span>

Choose a reason for hiding this comment

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

The Rust docs say "go to latest version", not "go to latest"; I realize we have limited space because this is in the sidebar, but I wonder if it might instead be possible to say something like View latest (v{{latestVersion}})?

@pdavies
Copy link
Author

pdavies commented Aug 6, 2024

Hello there, this PR slipped off my radar but I was reminded of it by this tweet: https://x.com/jskalc/status/1816920900660236370

I've pushed a change to address @halostatue's suggestion. Anything more you need from me here? Keen to get it in if possible!

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

Successfully merging this pull request may close these issues.

4 participants