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 version chooser JavaScript helpers derived from pydata-sphinx-theme #436

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

amotl
Copy link
Member

@amotl amotl commented Sep 4, 2023

About

Improve the version chooser component, in order not to produce invalid HTML links, which yield "404 Not Found" responses when following them, see #408 (comment).

Details

It will stop rendering any links into HTML at all, and use JavaScript instead, using relevant helpers from pydata-sphinx-theme. In this way, search engines will hopefully stop picking up the corresponding invalid links.

As a side effect, because the navigation process can be intercepted, users will not be redirected to invalid resources as well, but will be redirected to the documentation root instead.

Credits

@drammock and all contributors, thank you so much!

Copy link

@hlcianfagna hlcianfagna left a comment

Choose a reason for hiding this comment

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

Any chance we could do the tryUrl thing in the population phase and only offer versions for which the page exists?

@amotl
Copy link
Member Author

amotl commented Sep 5, 2023

Hi Hernan,

that definitively sounds like a great idea which I would like to explore in the future.

However, I am a bit hesitant on this right now, because it would create a strong binding across system boundaries. The reason is how the versioned artefacts are being deployed, is wired on behalf of corresponding Nginx configurations defined somewhere else.

For this and other reasons, I am currently leaning towards trusting @drammock and contributors on their decision to bring in a loose coupling on those matters, by resolving the target link at runtime, through JavaScript.

With kind regards,
Andreas.

Copy link

@drammock drammock left a comment

Choose a reason for hiding this comment

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

other than my more substantive comments below, I want to also address @hlcianfagna's question:

Any chance we could do the tryUrl thing in the population phase and only offer versions for which the page exists

This is probably OK as long as your total number of versions is small. I originally coded the PyData Sphinx Theme's version switcher that way, but a user pointed out the drawback that a network request will happen for each version in your switcher, on every page load instead of happening at most once per page view, only on-click.

That said, if you really want to omit entries for versions where the corresponding page doesn't exist (instead of falling back to the docs homepage for that version), then you're probably stuck with doing it for all versions on every page load.

Comment on lines 11 to 29
function checkPageExistsAndRedirect(event) {
const currentFilePath = `{{ pagename }}.html`,
tryUrl = event.target.getAttribute("href");
let otherDocsHomepage = tryUrl.replace(currentFilePath, "");

fetch(tryUrl, { method: "HEAD" })
.then(() => {
location.href = tryUrl;
}) // if the page exists, go there
.catch((error) => {
location.href = otherDocsHomepage;
});

// Cancel browser's native event handling.
// Prevent the browser from following the href of the clicked node,
// which is fine because this function takes care of redirecting.
return false;
}
Copy link

Choose a reason for hiding this comment

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

There were some recent changes to this function that you'll probably want to incorporate. See

https://github.com/pydata/pydata-sphinx-theme/blob/f6e1943c5f9fab4442f7e7d6f5ce5474833b66f6/src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js#L299-L316

  1. don't return false, use event.prevent_default() instead
  2. the fetch().then().catch() pattern you've copied here doesn't actually work as advertised. See the updated logic, which includes an if (head.ok) conditional to distinguish between "successful fetch of desired head (2xx response)" versus "successful fetch of the 404 page (4xx response)". The catch clause is still needed to handle genuine errors (like the fetch being blocked by CORS policy).

@amotl amotl force-pushed the amo/version-chooser-javascript branch from 28f9d90 to a31b30e Compare September 5, 2023 16:18
@amotl
Copy link
Member Author

amotl commented Sep 5, 2023

Dear Daniel,

thank you so much for the heads up. I've just updated the patch with your improved variant by adding a31b30e. Thank you again for your excellent work. 1

With kind regards,
Andreas.

P.S.: For some reason, I was not able to add an inline comment at #436 (comment). I think GitHub has a hiccup.

Footnotes

  1. If you are into sphinx-design in one way or another, feel free to inspect sphinx-design-elements from our pen. It is our humble variant of giving something back to the Sphinx community. Maybe it is also useful to you and your colleagues.

@amotl amotl requested a review from drammock September 5, 2023 16:33
> 1. don't return false, use event.prevent_default() instead
> 2. the fetch().then().catch() pattern you've copied here doesn't actually work as
>    advertised. See the updated logic, which includes an if (head.ok) conditional to
>    distinguish between "successful fetch of desired head (2xx response)" versus
>    "successful fetch of the 404 page (4xx response)". The catch clause is still needed to
>    handle genuine errors (like the fetch being blocked by CORS policy).

Thank you, @drammock.
@amotl amotl force-pushed the amo/version-chooser-javascript branch from a31b30e to d4ba6ef Compare September 6, 2023 06:06
@amotl amotl merged commit 9445d17 into main Sep 6, 2023
7 of 8 checks passed
@amotl amotl deleted the amo/version-chooser-javascript branch September 6, 2023 06:07
@amotl
Copy link
Member Author

amotl commented Sep 6, 2023

It works like a charm. For example, when navigating to the release notes page of a recent version 5.4 1, and then using the version chooser to navigate to version 5.3, where there is no 5.4.3.html.

image

Footnotes

  1. https://crate.io/docs/crate/reference/en/5.4/appendices/release-notes/5.4.3.html

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.

None yet

3 participants