Skip to content
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

Try to fix lang select #875

Closed
wants to merge 2 commits into from
Closed

Try to fix lang select #875

wants to merge 2 commits into from

Conversation

futurepaul
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change wasn't working for me on a fresh page and then I noticed the language state was changing from starting in the "en" form to "English" after selecting it in the ChooseLanguage component. The value label confusion was causing problems everywhere it seems! If we switch over to the shortName key I its a lot easier to make sense of these changes.

Comment on lines +23 to +28
function findLanguageByValue(value: string) {
return (
COMBINED_OPTIONS.find((language) => language.value === value) ??
EN_OPTION
);
}
Copy link
Collaborator

@benalleng benalleng Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is trying to find the Language object based on the value that is stored in localStorage or state which is acutally the shortName but as you'll se below we are searching for the form value based on the shortname already we don't need this function anymore and it can be removed

const [_chooseLanguageForm, { Form, Field }] =
createForm<ChooseLanguageForm>({
initialValues: {
selectedLanguage: findLanguageByValue(state.lang ?? "").value
Copy link
Collaborator

@benalleng benalleng Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is selecting the default value to display the user in the select form, but since we are using the shortName as the returned value of the selectField we should still use shortName here

selectedLanguage: state.lang ?? ""

Comment on lines +49 to +52
actions.saveLanguage(findLanguageByValue(f.selectedLanguage).value);

await i18n.changeLanguage(
findLanguageByValue(f.selectedLanguage).label
Copy link
Collaborator

@benalleng benalleng Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these values are saving the state and passing the new language to the i18n api and setting the new lang state so again they should be using the shortName and with the following code we actually do not need the findLanguageByValue() function

actions.saveLanguage(f.selectedLanguage);

await i18n.changeLanguage(f.selectedLanguage);

{({ value }) => (
<option
selected={field.value === value}
value={value}
Copy link
Collaborator

@benalleng benalleng Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are already iterating over the COMBINED_OPTIONS object we already have access to both value and shortName and can simply select the shortname to be sent to the i18n api while displaying the values to the user

<For each={COMBINED_OPTIONS}>
    {({ value, shortName }) => (
        <option
            selected={field.value === shortName}
            value={shortName}
        >
            {value}
        </option>
    )}
</For>

@benthecarman benthecarman deleted the try-to-fix-lang-select branch February 11, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants