-
Notifications
You must be signed in to change notification settings - Fork 912
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 language selection page (Fixes #5827) #6397
Add language selection page (Fixes #5827) #6397
Conversation
@media #{$mq-sm} { | ||
ul { | ||
@include grid-gap($spacing-lg $spacing-xl); | ||
display: grid; |
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.
Note for reviewer: as this page is so simple, I decided not to include a float/flex based fallback for browsers that don't support grid. Older browsers will just get a simple vertical list. I figured that this page is mainly for robots, so shouldn't matter too much. Happy to add a fallback if folks disagree, or we could update this page later to use the upcoming Protocol grid mixins.
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 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.
I checked all languages for sanity and that the locale homepage is online and all looks good
Ok, I'm going to say this is now ready for code review, removing the dnm label. |
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.
I agree that it's okay to just give a list to browsers that don't support grid. Good simplification. 💯
There's a couple language specific HTML tags and attributes to add here but it's looking good.
<section class="c-block-list"> | ||
<h2 class="c-block-list-title">America</h2> | ||
<ul> | ||
<li><a href="/cak/">Maya Kaqchikel</a></li> |
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.
Each language should have a lang
attribute associated with it, you can put it on the <li>
or the <a>
.
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.
<li lang="en-CA">
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.
Great suggestion, will do!
@media #{$mq-sm} { | ||
ul { | ||
@include grid-gap($spacing-lg $spacing-xl); | ||
display: grid; |
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.
👍
75da4eb
to
3b2bba2
Compare
Thanks @stephaniehobson updated |
R+ 🌎 |
Description
/locales/
.Note: will file an issue to research automation for this list post-launch.
Issue / Bugzilla link
#5827
Testing
Demo: https://bedrock-demo-agibson.oregon-b.moz.works/locales/