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

docs: add i18n overview & authoring documentation #9361

Merged
merged 5 commits into from
Jul 15, 2019
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Jul 12, 2019

@exterkamp deserves credit for writing this.
I've just spent some time reorganizing it and editing things.

Figured we'd just land this separate to keep the review separate from shane's great placeholders PR (#9114).

@paulirish paulirish requested review from exterkamp and removed request for brendankenny July 12, 2019 22:31
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM thanks for reorganizing this stream of conscious into a real README!

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.

LGTM!

boy do I want a snack now though :)


# Writing UIStrings with LHL

❗TODO(exterkamp): explain all the comments and where they go/what they become.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we leave this out/file issues for them?

Copy link
Member

Choose a reason for hiding this comment

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

imo I was going to handle that in #9114 once it was reimported 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

should we just block this PR on #9114? The implementation may still change until that lands...

Copy link
Member Author

Choose a reason for hiding this comment

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

let's land the doc. 9114 can always adjust anything as need be.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to do a review then, please, so I'm not just doing a review of it in #9114 after all

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops. Sorry just saw this right after i merged. Happy to take your review and i'll do whatever followups you're thinking

lighthouse-core/lib/i18n/README.md Outdated Show resolved Hide resolved
* To specify the description, use
* `@description <description>`
* To specify an example for an ICU replacement, use
* `@example {<ICU variable name>} <example for ICU replacement>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we leaving it at {variableName} replacement example then?

lighthouse-core/lib/i18n/README.md Outdated Show resolved Hide resolved

`{timeInMs, number, milliseconds}` is called _Complex ICU_ since the replacement is for complex numbers and uses the custom formatters in Lighthouse. The supported complex ICU formats are: `milliseconds`, `seconds`, `bytes`, `percent`, and `extendedPercent`.

These complex ICU formats are automatically given @example values during `yarn i18n`. Therefore, a normal description string can be used:
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️ dope!

lighthouse-core/lib/i18n/README.md Outdated Show resolved Hide resolved
@googlebot

This comment has been minimized.

@vercel vercel bot temporarily deployed to staging July 13, 2019 01:14 Inactive
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

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

Successfully merging this pull request may close these issues.

5 participants