-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
e1548f2
to
28f9d90
Compare
There was a problem hiding this 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?
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, |
There was a problem hiding this 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.
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; | ||
} |
There was a problem hiding this comment.
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
- don't
return false
, useevent.prevent_default()
instead - the
fetch().then().catch()
pattern you've copied here doesn't actually work as advertised. See the updated logic, which includes anif (head.ok)
conditional to distinguish between "successful fetch of desired head (2xx response)" versus "successful fetch of the 404 page (4xx response)". Thecatch
clause is still needed to handle genuine errors (like the fetch being blocked by CORS policy).
28f9d90
to
a31b30e
Compare
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, 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. 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.
a31b30e
to
d4ba6ef
Compare
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 Footnotes |
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!