Skip to content

Tweak language toggler - #2566

Merged
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-lang-toggler
Sep 26, 2019
Merged

Tweak language toggler#2566
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-lang-toggler

Conversation

@XhmikosR

@XhmikosRXhmikosR commented Sep 10, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#2545

@XhmikosRXhmikosR mentioned this pull request Sep 10, 2019
Closed
@XhmikosR

XhmikosR commented Sep 25, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@moshfeu please try this branch. Live preview: https://affectionate-almeida-a9f620.netlify.com/en/

It might need some further styling tweaks, but I think it's a big improvement already. In order for ESC to close the menu, well need more JS to handle this, but let's leave it for later.

/CC @nodejs/website (not sure if I can CC you guys, probably not)

@XhmikosR
XhmikosR marked this pull request as ready for review September 25, 2019 12:36
@XhmikosRXhmikosR changed the title Tweak lang togglerTweak language togglerSep 25, 2019
moshfeu
moshfeu approved these changes Sep 25, 2019
View reviewed changes

@moshfeumoshfeu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looking very good to me.
If they will take that PR (I hope so), it will worth to keep improve this component.

@XhmikosR

XhmikosR commented Sep 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe I should wrap the ul in a nav?

@XhmikosR
Tweak language toggler.
57e1ed5
* reduce the IDs usage
* reduce data attributes
* use classes
* keyboard navigation works now
@moshfeu

moshfeu commented Sep 25, 2019

Copy link
Copy Markdown

It's very opinion based but I spoke with accessibility expert some time ago and he actually didn't recommended to use nav because it's not a navigation action but sort of setting a configuration action. It's not a different page but the same page with different view.

The code also says something like that. Navigation will keep a record in the history, while the code replace the record:

nodejs.org/layouts/partials/header.hbs

Line 61 in 2026c6a

window.location.replace(window.location.pathname.replace(/\/[a-zA-Z-]+/, '/'+ newLocale))

@XhmikosR

XhmikosR commented Sep 25, 2019

Copy link
Copy Markdown
ContributorAuthor

That's something that needs to change BTW, see #2541.

@XhmikosR

XhmikosR commented Sep 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Anyway, I'll freeze the changes for now so that this can be reviewed. I also dropped one data attribute and used textContent which works for IE9+.

@moshfeu

moshfeu commented Sep 25, 2019

Copy link
Copy Markdown

That's something that needs to change BTW, see #2541.

I'm not sure :) But let's see what they will decide..

@Trott

Trott commented Sep 26, 2019

Copy link
Copy Markdown
Member

@nodejs/website Reviews?

3imed-jaberi
3imed-jaberi reviewed Sep 26, 2019
View reviewed changes

@3imed-jaberi3imed-jaberi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @XhmikosR,
I understand that you want to simplify the code but I think that these chnages aren't necessary, they are simple by nature ...

@XhmikosR

XhmikosR commented Sep 26, 2019

Copy link
Copy Markdown
ContributorAuthor

Please try and understand the changes. This makes the language toggler way more accessible and keyboard navigateable.

@moshfeu

moshfeu commented Sep 26, 2019

Copy link
Copy Markdown

@3imed-jaberi try to switch a language without the mouse, only keyboard.

3imed-jaberi
3imed-jaberi approved these changes Sep 26, 2019
View reviewed changes

@3imed-jaberi3imed-jaberi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For accessibility 👍

@alexandrtovmach

alexandrtovmach commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

Looks good to me

osk2
osk2 approved these changes Sep 26, 2019
View reviewed changes

@osk2osk2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Aissaoui-Ahmed
Aissaoui-Ahmed approved these changes Sep 26, 2019
View reviewed changes

@Aissaoui-AhmedAissaoui-Ahmed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great 💯

@Trott
Trott merged commit 78cd897 into nodejs:masterSep 26, 2019
@XhmikosR
XhmikosR deleted the master-xmr-lang-toggler branch September 27, 2019 05:48
@XhmikosRXhmikosR mentioned this pull request Sep 29, 2019
Merged
ghost pushed a commit that referenced this pull request Oct 1, 2019
@XhmikosRMaledong
The lang-toggler fixes(#2625)
822a40e
A minor regression from #2566
@NOUIYNOUIY mentioned this pull request Sep 20, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@celyescelyesAwaiting requested review from celyes
5 more reviewers
@osk2osk2osk2 approved these changes
@moshfeumoshfeumoshfeu approved these changes
@Aissaoui-AhmedAissaoui-AhmedAissaoui-Ahmed approved these changes
@alexandrtovmachalexandrtovmachalexandrtovmach approved these changes
@3imed-jaberi3imed-jaberi3imed-jaberi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Switch lang-picker-toggler to a button

7 participants

@XhmikosR@moshfeu@Trott@alexandrtovmach@osk2@Aissaoui-Ahmed@3imed-jaberi