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): import psuedo-locale json from TC #5726

Merged
merged 6 commits into from
Jul 27, 2018
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Jul 25, 2018

updated

  • ar-XB is psuedo right to left
  • en-XA is accented

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

cool cool cool

should we add these locales to the cli-flags + typings + index.js? if not, do we want to import them at all or was this just testing :)

also they seem to use _ while I was using - is there a best practice?

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.

also want to update https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/locales/index.js

and should

.choices('locale', [
'en-US', // English
'en-XA', // Accented English, good for testing
])

be based on Object.keys(require('...../locales/index.js'))?

@paulirish
Copy link
Member Author

also they seem to use _ while I was using - is there a best practice?

found another TC export script that has an option for hyphen rather than underscore. So this is a common thing. :)

Since our build script also prefers hyphen i guess we should go with that..

@paulirish paulirish force-pushed the importxcjson2 branch 2 times, most recently from 48d4206 to de7623d Compare July 25, 2018 22:43
@paulirish
Copy link
Member Author

running

lighthouse -GA --preset=perf http://example.com --locale=en-XA

I currently get this:
image

should I address it in this PR?

@patrickhulce
Copy link
Collaborator

I currently get this:
should I address it in this PR?

:/ ya the extra locales will be useless if we can't actually generate reports with them

@paulirish
Copy link
Member Author

ready for review.

lighthouse --preset=perf --locale=ar-XB http://example.com/
lighthouse --preset=perf --locale=en-XA http://example.com/

image

@paulirish
Copy link
Member Author

🏏 🏏

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!

@@ -58,7 +58,8 @@ describe('i18n', () => {
describe('#getRendererFormattedStrings', () => {
it('returns icu messages in the specified locale', () => {
const strings = i18n.getRendererFormattedStrings('en-XA');
expect(strings.passedAuditsGroupTitle).toEqual('P̂áŝśêd́ âúd̂ít̂ś');
expect(strings.passedAuditsGroupTitle).toEqual('[Þåššéð åûðîţš one two]');
expect(strings.scorescaleLabel).toEqual('[Šçöŕé šçåļé: one two]');
Copy link
Member

Choose a reason for hiding this comment

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

we should probably convert this file back to assert at some point rather than having a different assertion library just for this file

Copy link
Collaborator

Choose a reason for hiding this comment

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

nooooooooooooo 😢 assert is so sad compared to expect

@paulirish paulirish merged commit 1abdd17 into master Jul 27, 2018
@paulirish paulirish deleted the importxcjson2 branch July 27, 2018 02:07
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.

None yet

3 participants