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

fix(settings): Add default text to several l10n.getString calls #12861

Merged
merged 1 commit into from
May 12, 2022

Conversation

LZoog
Copy link
Contributor

@LZoog LZoog commented May 12, 2022

Because:

  • We must provide fallback text when pulling strings from Fluent since at the moment, we don't deploy en/en-US strings and rely on fallback text

This commit:

  • Adds the default text to several l10n.getString calls, updates some tests

fixes #12857
fixes #12858
fixes #12859


I started adding tests for all of these and then realized we were missing quite a few and some tests might take a bit longer to write. If you see something in particular feel free to call it out.

At the time of writing, without further investigation I don't know why these ever rendered correctly, we've been relying on fallback text for English for all of our strings and we even had tests to check for FTL IDs rendered rather than the English text.
Edit: see comment

This makes me eager to work on this spike, I think there's some l10n improvements we can make all around.

@LZoog LZoog requested a review from a team as a code owner May 12, 2022 17:15
expect(toggleButton).toHaveAttribute('aria-expanded', 'false');
expect(dropDown).not.toBeInTheDocument;
expect(dropDown).not.toBeInTheDocument();

fireEvent.click(toggleButton);
expect(toggleButton).toHaveAttribute('aria-expanded', 'true');
expect(dropDown).toBeInTheDocument;
Copy link
Contributor Author

@LZoog LZoog May 12, 2022

Choose a reason for hiding this comment

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

I spent a few minutes trying to get some of these .toBeInTheDocument tests changed to .toBeInTheDocument() but I don't know why they're failing. I'll file a follow up issue to check all of our toBeInTheDocument and toBeTruthy calls across React tests because we have a few like that for some reason, which makes the tests look like they're passing when they aren't necessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow up: #12868

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@LZoog Not sure why this broke?

l10n.getString(
'avatar-page-delete-error-2',
null,
'There was a problem deleting your profile picture'
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs a .

@pdehaan
Copy link
Contributor

pdehaan commented May 12, 2022

@LZoog Possibly this?

alertBar.error(l10n.getString('rk-remove-error'));

I've been trying to go through every result from git grep -n "l10n.getString" packages and verify it has 3 parameters, but wondering if there is a better way. Not sure how we could use a linter to enforce a method has 3 args (unless we do a lot of very scary AST work).
Or my cheaper solution of wrapping l10n.getString (which comes from @fluent/react's useLocalization() function) with a custom helper which just throws a hard error if the fallback string is undefined.

@LZoog
Copy link
Contributor Author

LZoog commented May 12, 2022

@LZoog Not sure why this broke?

@vbudhram I'm actually not sure why it was ever working. 😭 But I really wonder that too. It was always broken in Storybook. I don't see any changes in this train that would have caused it, or any changes on the l10n side.

@pdehaan Ah nice find, I'll add the default text for that one too.

Or my cheaper solution of wrapping l10n.getString (which comes from @fluent/react's useLocalization() function) with a custom helper which just throws a hard error if the fallback string is undefined.

This is a good idea if we want to keep relying on fallback text, I'm going to add a comment to that spike (those are becoming my famous last words).

Because:
* We must provide fallback text when pulling strings from Fluent since at the moment, we don't deploy en/en-US strings and rely on fallback text

This commit:
* Adds the default text to several l10n.getString calls, updates some tests

fixes #12857
fixes #12858
fixes #12859
@LZoog LZoog merged commit c62c0dd into main May 12, 2022
@LZoog
Copy link
Contributor Author

LZoog commented May 12, 2022

I think I've sorted out why this happened.

#12778 landed last train, which introduced a more correct Fluent strategy for Settings. I think prior to that fix, the entire page was rendering the strings from the en-GB bundle, since it was the closest available matching, as we don't have an en or en-US bundle and rely on fallback text for English (US, anyway) users. Fluent couldn't find the en or en-US bundle and didn't have fallback text supplied, so it returned the fluent IDs instead.

FWIW, we have had other conversations around making our l10n approach consistent across packages as well as improvements, including potentially not relying on fallback text at all for React applications (we'll weigh pros/cons), in this spike.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants