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

@XhmikosR XhmikosR commented Sep 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #2545

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

XhmikosR commented Sep 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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
@XhmikosR XhmikosR changed the title Tweak lang toggler Tweak language toggler Sep 25, 2019
moshfeu
moshfeu approved these changes Sep 25, 2019
View reviewed changes

@moshfeu moshfeu 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
Contributor Author

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
Contributor Author

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

@XhmikosR

XhmikosR commented Sep 25, 2019

Copy link
Copy Markdown
Contributor Author

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-jaberi 3imed-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 ...

@3imed-jaberi 3imed-jaberi requested review from a user, Aissaoui-Ahmed and celyes September 26, 2019 09:04
@XhmikosR

XhmikosR commented Sep 26, 2019

Copy link
Copy Markdown
Contributor Author

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-jaberi 3imed-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

@alexandrtovmach alexandrtovmach self-requested a review September 26, 2019 09:37
osk2
osk2 approved these changes Sep 26, 2019
View reviewed changes

@osk2 osk2 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-Ahmed Aissaoui-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:master Sep 26, 2019
@XhmikosR XhmikosR deleted the master-xmr-lang-toggler branch September 27, 2019 05:48
@XhmikosR XhmikosR mentioned this pull request Sep 29, 2019
Merged
ghost pushed a commit that referenced this pull request Oct 1, 2019
@XhmikosR
The lang-toggler fixes(#2625)
822a40e 
A minor regression from #2566
@NOUIY NOUIY mentioned this pull request Sep 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@celyes celyes Awaiting requested review from celyes

5 more reviewers

@osk2 osk2 osk2 approved these changes

@moshfeu moshfeu moshfeu approved these changes

@Aissaoui-Ahmed Aissaoui-Ahmed Aissaoui-Ahmed approved these changes

@alexandrtovmach alexandrtovmach alexandrtovmach approved these changes

@3imed-jaberi 3imed-jaberi 3imed-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