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.

Sign up for GitHub

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

Jump to bottom

Add language selection dropdown #994

Merged
merged 19 commits into from
Jul 26, 2024
Merged

Add language selection dropdown #994

merged 19 commits into from
Jul 26, 2024

Conversation

ignotus666
Copy link
Contributor

@ignotus666 ignotus666 commented Jun 21, 2024 •
edited
Loading

Short description of changes
Adds a language selection dropdown to the website. The website also detects what language the user's browser is in and initially displays itself in that language.

Scripts automating the addition of new languages have been updated to support adding the full language name + language code.

It also includes the changes from #988 and #990 so as to avoid conflicts between PRs and include all the changes here.

TODO after merging this:

Context: Fixes an issue? Related issues
Fixes #977, #986 and #992

Status of this Pull Request
Should be ready pending a review.

What is missing until this pull request can be merged?
Would be good if some more people can test it.

IMPORTANT: Weblate stuff should be merged first.

Does this need translation?

No.

@ignotus666 @gilgongo @github-actions
Lang select (#11)
18a7af9 
* Initial replacements testing OK so far

* Add language selection dropdown

* Don't store lang preference

* Align dropdown to the right. Remove unneeded code from fw.css

* Make dropdown scrollable

* Clean/move/improve css styling, add icon, lang sel. outline

* Remove unneeded css

* Show full lang names, automate adding langs

* AUTO: Add new language: (eu) #4

* Revert "AUTO: Add new language: (eu) #4"

This reverts commit 301c948.

* Correction

* AUTO: Add new language: (Basque) #5

* Update add-lang.yml

* AUTO: Add new language: (English (American)) #6

* Correction

* Revert language additions

* Correction

* Correction

* Update

* Update

* Add check

* Update error message

* Revert lang code hyphens

* Add changes by @gilgongo

* Combine with gilgongo's stuff

* Correct title check

* Correct pt-BR language codes

* Revert lang code changes

---------

Co-authored-by: Gilgongo <[email protected]>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@ignotus666
Copy link
Contributor Author

ignotus666 commented Jun 21, 2024 •
edited
Loading

As commented in #993, the check fails because of lang code mismatches that will be fixed after this PR is merged. To test the language dropdown version of the website, links to working artifacts will be posted in comments below.

@pljones pljones added bug Something isn't working enhancement labels Jun 23, 2024
@pljones pljones added this to the Release 3.11.0 milestone Jun 23, 2024
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

In general, I see the benefits. The redirect would be a new feature and it seems to be possible to click the picker to get back to another language if it fails.

ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
_includes/langselect.html Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
assets/img/lang-icon.png Outdated Show resolved Hide resolved
ann0see
ann0see reviewed Jun 23, 2024
View reviewed changes
sitemap.xml Show resolved Hide resolved
@ann0see
Copy link
Member

ann0see commented Jun 23, 2024 •
edited
Loading

What would a classic html select/option box with some styling be as alternative? It would be much better integrated in mobile OS and be standard compliant:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

@ann0see
Copy link
Member

ann0see commented Jun 23, 2024

I'd like to be part of the final review on this one.

@pljones
Copy link
Contributor

pljones commented Jun 24, 2024 •
edited
Loading

Regarding avoidance of reliance on javascript:

  • have the default site written with the "Language Select" as a standard popup (or even whole page) that has a link for each language that takes you to the current url in that language
  • have an onLoad script that replaces that with the "Language Select" widget and also runs the auto-detect stuff

That should be pretty transparent to the user - the benefit of the auto-detect (and then the widget having local storage) would be there for those who are happy to have javascript and local storage enabled but the functionality wouldn't be lost for other users.

@pljones pljones linked an issue Jun 25, 2024 that may be closed by this pull request
Language selection dropdown #992
Closed
@ignotus666
Copy link
Contributor Author

@ann0see, @pljones: I have a new version that doesn't do language detection nor uses javascript, and the css styling is moved to its own langsel.css file. The code is way more simplified. Other than not detecting the language automatically, it behaves exactly the same. So subject of course to a review, would you be happier if I replaced the current version with that?

@ann0see
Copy link
Member

ann0see commented Jun 26, 2024

Yes.

@gilgongo
Copy link
Member

gilgongo commented Jun 26, 2024 •
edited
Loading

We'll need to verify the accessibility of this at some point too. Not sure how best to do that though (I see there's some Aria tags though).

@ann0see
Copy link
Member

ann0see commented Jun 26, 2024

As I said above, I'd assume that a select box is more accessible. But IDK if clicking easily works without JS. We could use a form.

@softins
Copy link
Member

softins commented Jul 15, 2024

Thanks, I'll give it a go.

@ann0see
Copy link
Member

ann0see commented Jul 15, 2024

language duplication

The selection box shows up twice. And I don't think it looks nice in the footer.

@ann0see
Copy link
Member

ann0see commented Jul 15, 2024

@ignotus666 also please check with #994 (comment) multiple issues are showing up in the w3c validator.

@ignotus666
Copy link
Contributor Author

The selection box shows up twice. And I don't think it looks nice in the footer.

Oops, a stray line got left in there (but not in this PR). Fixed version is here.

also please check with #994 (comment) multiple issues are showing up in the w3c validator.

What file am I supposed to upload there, langselect.html? It also spits out a bunch of warnings and errors with the current (next-release) version.

@ann0see
Copy link
Member

ann0see commented Jul 16, 2024 •
edited
Loading

Ok. No you should upload the compiled index.html from the artifact and check for new errors then.

@ignotus666
Copy link
Contributor Author

Right. So the main complaint (that differs from the next-release errors) seems to be that "canonical" in lang="canonical" is more than 8 characters long... How are we supposed to get around that?

@gilgongo
Copy link
Member

gilgongo commented Jul 16, 2024 •
edited
Loading

My understanding is that lang= needs to be hreflang="[langcode]" (eg hreflang="ko-KR") and that for links outside of the <head> section we don't need to have rel="canonical" and rel="alternate" as you point out (but could have rel="[something else]" if needed I guess). But I don't know what the difference is here between lang and hreflang... There seems to be some Google preference for hreflang.

@ignotus666
Copy link
Contributor Author

ignotus666 commented Jul 16, 2024 •
edited
Loading

I traced the problem back to nav.html. I think it's sorted now. Link to artifact. Tell me if this one seems ok and then I'll add the changes to this PR.

@gilgongo
Copy link
Member

That looks good. I don't know whether the links in the nav to things like "/wiki/Getting-Started" also need to have hreflang= in them but perhaps that's not needed with the rel= stuff in the head area.

@ignotus666
Copy link
Contributor Author

ignotus666 commented Jul 16, 2024 •
edited
Loading

Here's what I found:

Use rel="canonical" in navigation links for the primary version of a page.
Reserve hreflang for <link> elements in the <head> to specify alternate language versions of pages.

This was referenced Jul 20, 2024
Closed
Closed
ann0see
ann0see reviewed Jul 21, 2024
View reviewed changes
_layouts/mainhomepage.html Outdated Show resolved Hide resolved
@pljones pljones mentioned this pull request Jul 22, 2024
Closed
60 tasks
@ignotus666
Copy link
Contributor Author

ignotus666 commented Jul 24, 2024 •
edited
Loading

Latest version: artifact.

ann0see
ann0see reviewed Jul 25, 2024
View reviewed changes
_includes/headtags.html Outdated
{% endfor %}
<link rel="alternate" hreflang="x-default" href="{{ site.url }}/{{ site.default_lang }}{{ page.url }}">
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this is not correct: it will produce /en/ but there is no /en/ subpage.

Suggested change
<link rel="alternate" hreflang="x-default" href="{{ site.url }}/{{ site.default_lang }}{{ page.url }}">
<link rel="alternate" hreflang="x-default" href="{{ site.url }}/{{ page.url }}">

is probably correct.

Copy link
Member

Choose a reason for hiding this comment

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

I hope the whole canonical/alternate thing is correct then.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see Google says "The new x-default hreflang attribute value signals to our algorithms that this page doesn't target any specific language or locale and is the default page when no other page is better suited."

Copy link
Member

Choose a reason for hiding this comment

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

Which we could argue is ok for en.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that this is not correct: it will produce /en/ but there is no /en/ subpage.

Yes, you're right.

@ann0see ann0see merged commit 247363b into jamulussoftware:next-release Jul 26, 2024
1 check failed
@ann0see
Copy link
Member

ann0see commented Jul 26, 2024

Ok. Thanks! It was a long ride but now hopefully it's all good. We'll probably need to check the site before publication for e.g broken links and so.

@ignotus666
Copy link
Contributor Author

It was a long ride

You can say that again... ;) . Right, now before unlocking Weblate or merging anything else here there are a couple of things that need doing (see description of this PR). Hopefully I'll have some time tonight to put it together.

@ignotus666 ignotus666 deleted the lang-select2 branch July 27, 2024 20:43
@gilgongo
Copy link
Member

gilgongo commented Aug 23, 2024 •
edited
Loading

@ignotus666 I'm looking the the rel=canonical/alternate stuff and I'm not sure it's correct. Shall I open a PR with a couple of tweaks to headtags.html? If so, would that be against release or next-release?

I also notice a rel="canonical" in the 1-*-index.md files which probably shouldn't be there - how best to remove that? that's in wiki/en/misc/1-index.md now I take it?

@ann0see
Copy link
Member

ann0see commented Aug 23, 2024

In theory next-release. But I'd rather wait until the translations have settled even though it shouldn't have conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilgongo gilgongo gilgongo approved these changes

@pljones pljones pljones left review comments

@ann0see ann0see ann0see approved these changes

@trebmuh trebmuh trebmuh approved these changes

Assignees

@ignotus666 ignotus666

Labels
bug Something isn't working enhancement
Projects
Tracking
Status: Done
Milestone
Release 3.11.0
Development

Successfully merging this pull request may close these issues.

Language selection dropdown
6 participants
@ignotus666 @ann0see @pljones @gilgongo @softins @trebmuh