-
Notifications
You must be signed in to change notification settings - Fork 28
3349: Improve language selection #3622
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
base: main
Are you sure you want to change the base?
Conversation
| expect(getAllByText('English')[0]?.closest('a')).toHaveAttribute('href', poi0.availableLanguages.en) | ||
| expect(getAllByText('Deutsch')[0]?.closest('a')).toHaveAttribute('href', poi0.availableLanguages.de) | ||
| // Pathname is not correctly updated, therefore the pathname does not include the slug | ||
| expect(getAllByText('اَللُّغَةُ اَلْعَرَبِيَّة')[1]?.closest('a')).toHaveAttribute('href', '/augsburg/ar/locations') |
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.
Does anybody know why we're testing for the Arabic translation when there isn't one in the PoiModelBuilder?
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 think English, German and Arabic where just the three default languages and its always good to test with an RTL language/different alphabet as well (obviously doesn't matter in this case but I think it also doesnt really hurt?).
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 would say that this is a poorly written test, that half-tests something that doesn't work (I also don't really understand the comment, how is the pathname not correctly updated? Is that because there is no Arabic translation in the poi?) If we want to test for Arabic, then I think we should add Arabic test data to the test 🤷
|
Nice! I think we should carefully consider this, perhaps we can combine this with the previous design + the search box somehow? I think without German aliases for the languages this perhaps even worsens the language selection. As e.g. counselor, I would not want to have to type Farsi in Farsi in another alphabet to find the language. While its still possible to just select the language from the list, I think it was easier/more overseeable before. |
46ffffe to
f9deab9
Compare
|
🤔 check this out https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DisplayNames#language_display_names |
6a3d29a to
b19b980
Compare
Thank you, the Intl object is really very cool! Unfortunately, it's not fully implemented in react-native, and the DisplayNames are one of the things that aren't 😢 |
check this out https://www.npmjs.com/package/@formatjs/intl-displaynames?activeTab=readme what do you think of this?
|
|
Also we have https://github.com/unicode-org/cldr-json/tree/main/cldr-json/cldr-localenames-full we can take what we need (each folder has languages.json). |
|
I played around a bit with the polyfills from FormatJS and got a first draft running. Since getting the actual size of the app when downloading from the app store requires uploading it to said app store (there's lots of additional caching also between different apps), these absolute numbers are not reliable, but I assume that the relative change in size is somewhat accurate, at least on iOS. For a release version on iOS, it went up from 59.1 MB to 59.5 MB. For a debug version on Android, the size stayed at 234.3 MB, I'll check if the polyfill is necessary there at all (when I'm back from vacation in a week). Either way, I would say that it's worth adding the polyfill in. Other opinions? |
Sounds good, perhaps we can double check the actual size change in the stores afterwards again. This polyfill only adds the language names though, correct? So all the other intl stuff that does not yet work in react-native does not get fixed by this? |
The polyfill also needs Intl.Locale (polyfill and real deal) but it's not enough to replace the other stuff. It's a good idea to check on polyfills for those, I'll create a ticket for it. |
2622fc3 to
cf8e028
Compare
56e9383 to
e5e3643
Compare
|
Alright, this is only the changes for the web, I'll open a separate PR branch off of the redesign for the native changes, including the polyfills. |
bahaaTuffaha
left a comment
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.
Nice work! ✨
Tested on Chrome and Firefox ✅
Left some comments...
f1sh1918
left a comment
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.
Works fine 👍
Just have some general question to the auto complete.
It should also be possible to use that mobile with some styling adjustments
Short Description
I rewrote the language selection to use Mui Autocomplete, in order to make it easier to find languages when there are a lot of them.
Proposed Changes
CityContentHeaderif there are no languages (which shouldn't be the case)Dropdownwhich wasn't used anywhere elseSelectorItemModelwhich wasn't used anywhere elseSide Effects
Testing
Change the language on lots of pages
Resolved Issues
Fixes: #3349