Feat - Move user languages to top of the list #4853
Conversation
| const availableLocales = useAvailableLocales() | ||
| const availableLocalesWithNames = useNativeNameAvailableLocales() | ||
| let availableLocalesWithNames = useNativeNameAvailableLocales() |
There was a problem hiding this comment.
Generally we try to avoid using let for variable declaration when possible. This can be refactored like below so you don't need to use let:
const initialAvailableLocalesWithNames = useNativeNameAvailableLocales()
const getAvailableLocalesWithNames = () => {
// Get user languages to top of the list
if (userLanguages.length > 0) {
const userLanguagesWithNames = userLanguages.map(lang =>
initialAvailableLocalesWithNames.find(({ code }) => code === lang)
)
return userLanguagesWithNames.concat(
initialAvailableLocalesWithNames.filter(
item => !userLanguages.includes(item.code)
)
)
}
return initialAvailableLocalesWithNames
}
const availableLocalesWithNames = getAvailableLocalesWithNames()
function getLocaleWithName(locale: string) {
return availableLocalesWithNames.find(({ code }) => code === locale)
}There was a problem hiding this comment.
One change I just made is to check if the userLanguages array is not empty so we don't run the if-statement even when the array is empty. Empty arrays are truthy.
There was a problem hiding this comment.
We all do. It is just CPU cycles against RAM :) That refactoring is a good idea of course. Implemented with the initial value fix.
Done
| @@ -82,9 +91,11 @@ const LocalizationSelectComplex = ({ locale, onLocaleChange }: Props) => { | |||
| key={item} | |||
| className={classNames('list-item-wrapper', { | |||
| highlighted: index === highlightedIndex, | |||
| })}> | |||
| })} | |||
| style={{ borderBottomColor: userLanguages.length > 0 && index === userLanguages.length-1 ? "#333" : "#f3f2f0" }} | |||
There was a problem hiding this comment.
Could you use the classNames library to add a class that does? Also use the colors we have defined in vars.css in the css for this component
There was a problem hiding this comment.
I nearly always used CSS libraries. classNames is a simple and powerful one, especially conditionals!
Done
|
Thanks for your contribution @HarikalarKutusu ❤️ 🚀 I've left two minor comments |
|
@moz-rotimib, there is one weird behavior I encounter, which also exists in original. |
@HarikalarKutusu I got this too. I was able to solve it with useEffect like you mentioned. This is what worked for me. You can test and check on your end. const initialSelectedItem =
userLanguages.length > 0
? availableLocalesWithNames[0].code
: localWithName.code
React.useEffect(() => {
selectItem(initialSelectedItem)
}, [initialSelectedItem])I also removed |
@moz-rotimib: Above one enforces user's first language :) The following works as desired - with added useEffect... I need to check for locales without native names, probably |
@HarikalarKutusu all good 🚀 once the changes are made I'll test locally and approve |
|
It seems OK on my side, you can test now... |
|
@HarikalarKutusu it seems the This is because in the test we are asserting that onLocaleChange is called only once when the user localization dropdown is selected. In reality it's called twice because we first call So to fix this text, I advise that we reset the mock after the component is rendered and then make the assertion as usual. Here's a code snippet to show what I mean. it('should call onLocalChange with a new locale when changed', async () => {
const onLocalChangeMock = jest.fn()
renderWithProviders(
<LocalizationSelectComplex onLocaleChange={onLocalChangeMock} />
)
// resets the mock because we call onLocaleChange once on render
onLocalChangeMock.mockClear() ----> // RESETS THE MOCK
// pick an option in the select box
userEvent.selectOptions(
screen.getAllByLabelText('Choose language/localization')[1],
screen.getByRole('option', { name: '臺語' })
)
expect(onLocalChangeMock).toBeCalledWith('nan-tw')
expect(onLocalChangeMock).toBeCalledTimes(1)
})Let me know what you think |
|
I have Jest extension on CV Code in this new dev environment but it gives a long list warnings and errors. I need to properly configure it to catch these locally. Not ideal that it renders twice because of useEffect, but we would not get it right otherwise... Implemented what you suggested, let's see how it tests. |
|
Looks good 🚀 will merge and deploy in the next release. Thanks |
|
Great 👍 Thank you for your help 🥇 |
This implements the idea presented in #4734
For logged in users who have languages set in their profiles, the user language codes are passed to the language selector component, where the list is re-organized.
When not logged-in or no user languages selected, it behaves normally.
Screenshot:
