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

Refactor switchers.js #225

Merged
merged 23 commits into from
Oct 29, 2024
Merged

Refactor switchers.js #225

merged 23 commits into from
Oct 29, 2024

Conversation

AA-Turner
Copy link
Member

  • Modernise (remove obsolete polyfills, update to use e.g. arrow functions, use Map over object literals)
  • Improve documentation (add JSDoc comments)
  • Improve handling for local browsing (file: URIs)
  • Improve core redirection logic
  • Improve readability (prefer named functions to anonymous, give more variables names)
  • Create switchers from DOM nodes rather than less-secure parsing of HTML strings

A

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

I haven't checked everything in detail, but what I saw LGTM.

templates/switchers.js Outdated Show resolved Hide resolved
if (_IS_LOCAL) return;

const selected_version = event.target.value;
// English has no language prefix.
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but I read through PEP 545 – Python Documentation Translations this weekend and it says:

http://docs.python.org/en/ will redirect to http://docs.python.org/.

Currently /en/ is 404, I wonder if we should add this redirect or if it was decided to be unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also seen that, though I never got around to updating the PEP. I think that we should remove it from the PEP as something that was never implemented, rather than adding another set of redirects to maintain.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how many people try /en/, but fine by me. And the PEP should probably Process/Active rather than Process/Final.

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +121 to +134
// 1. The current page in the current language with the new version
// 2. The current page in English with the new version
// 3. The documentation home in the current language with the new version
// 4. The documentation home in English with the new version
_navigate_to_first_existing([
window.location.href.replace(_CURRENT_PREFIX, new_prefix),
window.location.href.replace(_CURRENT_PREFIX, new_prefix_en),
new_prefix,
new_prefix_en,
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would be better to stick in the current language, even if that means going back to the homepage?

It might be surprising to switch to a completely different language set; as you navigate further everything is English and not your chosen language.

And I think different languages should nearly always have the same structure and same pages for any given version? (Although with some untranslated pages.)

That would mean:

Suggested change
// 1. The current page in the current language with the new version
// 2. The current page in English with the new version
// 3. The documentation home in the current language with the new version
// 4. The documentation home in English with the new version
_navigate_to_first_existing([
window.location.href.replace(_CURRENT_PREFIX, new_prefix),
window.location.href.replace(_CURRENT_PREFIX, new_prefix_en),
new_prefix,
new_prefix_en,
// 1. The current page in the current language with the new version
// 2. The documentation home in the current language with the new version
// 3. The current page in English with the new version
// 4. The documentation home in English with the new version
_navigate_to_first_existing([
window.location.href.replace(_CURRENT_PREFIX, new_prefix),
new_prefix,
window.location.href.replace(_CURRENT_PREFIX, new_prefix_en),
new_prefix_en,

But I'm really not sure. Might be worth asking people who regularly uses the docs in another language?

Copy link
Member Author

@AA-Turner AA-Turner Oct 29, 2024

Choose a reason for hiding this comment

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

Shall we create an issue for this? The intent in this PR was for as few behavioural changes as possible (the ideally none!).

Although, the current order does have some logic, given that a reader probably prefers to have the information (which can be translated via browser tools, Google, etc) rather than always being sent to the homepage for a language that doesn't have much translation coverage.

(Also, this order was chosen by a French-speaker!)

A

Copy link
Member

Choose a reason for hiding this comment

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

@AA-Turner
Copy link
Member Author

I'll merge this tomorrow so that I can keep an eye on builds (unless anyone would prefer we delay)

A

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

I'll merge this tomorrow so that I can keep an eye on builds (unless anyone would prefer we delay)

A

That's fine, we can easily adjust the order later if needed. Thanks!

@AA-Turner
Copy link
Member Author

Force-push diff (added logging for _navigate_to_first_existing)

@AA-Turner AA-Turner merged commit 757acb1 into python:main Oct 29, 2024
5 checks passed
@AA-Turner AA-Turner deleted the switchers/refactor branch October 29, 2024 16:58
@hugovk
Copy link
Member

hugovk commented Oct 30, 2024

I just noticed, likely this PR has resulted in this bit of CSS no longer applying for smaller screens:

@media (max-width: 1023px) {
    .version_switcher_placeholder > select {
        height: 100%;
    }
}

And so the version switcher (in 3.12-3.14) doesn't fill the top bar:

image

@AA-Turner
Copy link
Member Author

#233 to fix.

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.

3 participants