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

core(i18n): always use formatted strings for extension popup #5761

Merged
merged 3 commits into from
Aug 2, 2018

Conversation

paulirish
Copy link
Member

similar to #5727

fixes this:
image

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM with a note about a possible better test

@@ -92,4 +113,13 @@ describe('Lighthouse chrome popup', function() {
assert.equal(titleText, 'Lighthouse');
assert.equal(urlText, 'http://example.com');
});


// Kinda lame as the mocked data is already good.
Copy link
Member

Choose a reason for hiding this comment

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

I guess a unit test (which would still be trivial but at least testing something) would be that the categories passed back are what are in the labels and values of the check boxes? So maybe keep the array passed back by the mocked getDefaultCategories in a top level variable and loop over it here to verify that all the members are there?

(rather than checking for the lack of ICU messages, which doesn't seem possible since the mock doesn't supply any of them)

@paulirish
Copy link
Member Author

trashed the old test and wrote the one you described. sg?

@paulirish paulirish added the i18n internationalization thangs label Aug 2, 2018
@brendankenny
Copy link
Member

sg?

yep, LG!

@brendankenny brendankenny merged commit 1fb885b into master Aug 2, 2018
@brendankenny brendankenny deleted the extpopupformatted branch August 2, 2018 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants