-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Language picker #12006
Language picker #12006
Conversation
fix logic to fetch a display name for each locale in the LanguagePicker, both in the language of the current locale (source) and the language of that locale choice (target). Uses internal i18n translations if available, falling back to native Intl package if not available, and falling back to English name for locales that are not found elsewhere. Updates translations.ts to add "page-translations" next to "common" for all pages, and updates `existsNamespace` "primary namespace" argument to look at the index one higher (except for /languages page and pages that use "common" as primary ns)
add handleClose logic
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit first data fetch of translation progress
495e383
to
0e7706c
Compare
0e7706c
to
ac61dfd
Compare
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.
Second part of the review.
update isOpen/onOpen/onClose disclosure handling, export {t} from hook
One thing I noticed that I'll note as a potential bug: This is currently set up to auto-focus on the filter input when the menu is opened. It is also set up where pressing "Enter" from the filter input will automatically navigate to the first filter option available. If a user clicks this menu from ANY non-English page, and then they press "Enter", they will automatically be taken to the English version of the site, since that always be the first result with an empty filter query... This may be easier to switch than we'd like, since it may be triggered by accident. I'm not sure I see this as a blocker myself, but open to thoughts or suggestions on how we can keep this convenience without potentially inhibiting the UX. We could ditch the auto-focus, and/or the "Enter" shortcut to trigger first result.... Personally think I'd be more in favor of removing the "Enter" trigger (since user can still press "Down -> Enter"); or only allowing this shortcut ONLY IF the filter query is not empty. |
@wackerow agreed. Given that we have changed the default Menu behavior, I think we should disabled the default "Enter" trigger and keep our implementation since now it is not just a list, it is more of a form where the "Enter" key plays a different role. |
resolves RTL positioning bug
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 job @wackerow! a lot of work done here. Overall it works really well.
Small issue (or feature 🤔?? xD): the user can get a weird experience when scrolling and hovering the items. If you point to the top of the list, you might get some undesired scrolling to the top of the list. Don't expect this to be an easy fix so no bother on investigating this now. Just saying in case we want to try to fix it later.
https://github.com/ethereum/ethereum-org-website/assets/468158/6d599ce8-707e-4145-892c-cc4a18987fc0
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.
Comment for next iterations: is it possible to add a new step to minimize this as much as we can? seems that there is room for improvements here.
This file will get bundled in the main app.js file that all the pages need to load impacting the overall client load performance.
useEventListener("keydown", (e) => { | ||
if (e.key !== "\\") return | ||
e.preventDefault() | ||
inputRef.current?.focus() |
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.
Couldn't we simplify this autofocus when the menu is opened, not just when the keydown
is pressed? I say this because we should also autofocus the input when the user clicks the button.
There is a pretty standard user behavior where you click the button and then just start typing. You can't do that right now.
I think this was the original idea and I saw it working before. Just calling it out.
onKeyDown={(e) => { | ||
if (e.key === "Tab" || e.key === "\\") { | ||
e.preventDefault() | ||
;(e.shiftKey ? inputRef : footerRef).current?.focus() |
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.
This is cool, but I have a feeling that most users will never notice that it exists. For future iterations, we could add a help icon/tooltip or a shortcuts summary that explains this.
onChange={(e) => setFilterValue(e.target.value)} | ||
onBlur={(e) => { | ||
if (e.relatedTarget?.tagName.toLowerCase() === "div") { | ||
e.currentTarget.focus() |
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.
Couldn't understand what this is for, could you explain? (we could add a comment)
<Box | ||
position="relative" |
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.
Not a blocker: wondering if we could treat this as an actual form
<Box | |
position="relative" | |
<Box | |
as="form" | |
position="relative" |
We could have some a11y wins when we submit the form (also, would remove the need of the custom onKeyDown
handler).
const navLangs = typeof navigator !== "undefined" ? navigator.languages : [] | ||
|
||
// For each browser preference, reduce to the most specific match found in `locales` array | ||
const allBrowserLocales: Lang[] = navLangs | ||
.map( | ||
(navLang) => | ||
locales?.reduce((acc, cur) => { | ||
if (cur.toLowerCase() === navLang.toLowerCase()) return cur | ||
if ( | ||
navLang.toLowerCase().startsWith(cur.toLowerCase()) && | ||
acc !== navLang | ||
) | ||
return cur | ||
return acc | ||
}, "") as Lang | ||
) | ||
.filter((i) => !!i) // Remove those without matches | ||
|
||
// Remove duplicate matches | ||
const browserLocales = Array.from(new Set(allBrowserLocales)) |
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.
Refactor idea: move all this logic to a util function.
const navLangs = typeof navigator !== "undefined" ? navigator.languages : [] | |
// For each browser preference, reduce to the most specific match found in `locales` array | |
const allBrowserLocales: Lang[] = navLangs | |
.map( | |
(navLang) => | |
locales?.reduce((acc, cur) => { | |
if (cur.toLowerCase() === navLang.toLowerCase()) return cur | |
if ( | |
navLang.toLowerCase().startsWith(cur.toLowerCase()) && | |
acc !== navLang | |
) | |
return cur | |
return acc | |
}, "") as Lang | |
) | |
.filter((i) => !!i) // Remove those without matches | |
// Remove duplicate matches | |
const browserLocales = Array.from(new Set(allBrowserLocales)) | |
const browserLocales = getBrowserLocales(locales) |
|
||
import { DEFAULT_LOCALE } from "@/lib/constants" | ||
|
||
const data = progressData as ProjectProgressData[] |
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'd keep using progressData
for its name, much clearer than the generic data
.
We do have resolveJsonModule
in our tsconfig so, this data should be typed already. If we need to force the ProjectProgressData
type, we could do that in our global.d.ts
using declare module
.
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.
LFG!! 🚀 🚀
Thank you for noting these remaining issues! Will take note of these, but gonna bring it in in the meantime. Will merge these into the Radix-UI nav-menu PR (#12125) to clear conflicts |
Description
Adds a new language selection menu to replace the "Languages" link in the navigation bar (or mobile menu), using Chakra-UI Menu component.
Translation callouts
Data fetching
@crowdin/crowdin-api-client
, saving results to a data fileScreenshot
Preview link
Related issue
Related PRs