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.

Sign up for GitHub

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

Jump to bottom

docs(aio): updated i18n guide and example #19975

Merged
merged 1 commit into from Nov 2, 2017
Merged

docs(aio): updated i18n guide and example #19975

merged 1 commit into from Nov 2, 2017

Conversation

ocombe
Copy link
Contributor

@ocombe ocombe commented Oct 27, 2017 •
edited

PR Type

What kind of change does this PR introduce?

[x] Documentation content changes

Target: v5.0.0 docs

What is the current behavior?

I18n guide is outdated

Issue Number: #19510

What is the new behavior?

Updated i18n guide for Angular v5+ and cli 1.5+.

Does this PR introduce a breaking change?

[x] No

Things that we could add later

  • how to create multiple builds
  • how to use the new locale API for libraries/generic components
  • how to change base href
  • how to update meta data and lang attribute (for html tag)

Foxandxss
Foxandxss previously requested changes Oct 27, 2017
View reviewed changes
aio/content/examples/i18n/doc-files/main.2.ts Outdated
@@ -0,0 +1,22 @@
// #docregion
import {enableProdMode, TRANSLATIONS, TRANSLATIONS_FORMAT} from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

This and all the other import {} needs spaces, AKA { enableProdMode } and not {enableProdMode}

Copy link
Member

Choose a reason for hiding this comment

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

Of course, in all files

@ocombe ocombe force-pushed the feat/aio-i18n-guide-v5 branch 2 times, most recently from 294f17c to 6b6e9b5 Compare October 27, 2017 10:24
@ocombe ocombe requested a review from Foxandxss October 27, 2017 10:24
This was referenced Oct 27, 2017
Closed
Closed
@ocombe ocombe force-pushed the feat/aio-i18n-guide-v5 branch 2 times, most recently from 36f5ddc to b4a3d33 Compare November 1, 2017 09:16
@angular angular deleted a comment from mary-poppins Nov 1, 2017
@mary-poppins
Copy link

You can preview b709810 at https://pr19975-b709810.ngbuilds.io/.

@ocombe ocombe requested review from gkalpak and removed request for wardbell, Foxandxss and juleskremer November 1, 2017 23:02
@angular angular deleted a comment from mary-poppins Nov 1, 2017
@mary-poppins
Copy link

You can preview 9ce17d9 at https://pr19975-9ce17d9.ngbuilds.io/.

@ocombe ocombe added target: major This PR is targeted for the next major release and removed PR target: TBD labels Nov 1, 2017
gkalpak
gkalpak approved these changes Nov 2, 2017
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM as soon as Travis is happy (except for the guide content, which has been approved by @jenniferfell).

aio/content/examples/i18n/doc-files/app.component.html
@@ -29,4 +29,3 @@ <h1 i18n="@@introductionHeader">Hello i18n!</h1>
<!--#docregion i18n-title-->
<img [src]="logo" title="Angular logo">
<!--#enddocregion i18n-title-->
Contact GitHub API Training Shop Blog About
Copy link
Member

Choose a reason for hiding this comment

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

😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, no idea why we had this

aio/content/examples/i18n/src/app/app.component.html
<!--#docregion i18n-select-->
<span i18n>The hero is {gender, select, m {male} f {female}}</span>
<span i18n>The author is {gender, select, m {male} f {female} o {other}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

I think @naomiblack once said that using gender in examples is tricky and maybe not a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the example used previously, I just added other, but you're right, maybe I can find another example

aio/content/examples/i18n/src/app/app.component.ts Outdated
}
male() { this.gender = 'm'; }
female() { this.gender = 'f'; }
other() { this.gender = 'o'; }
Copy link
Member

Choose a reason for hiding this comment

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

Either align all three, or none 😁

aio/content/examples/i18n/e2e/app.e2e-spec.ts Outdated
@@ -0,0 +1,48 @@
'use strict'; // necessary for es6 output in node
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't TypeScript add that?

aio/content/examples/i18n/e2e/app.e2e-spec.ts Outdated

import { browser, element, by } from 'protractor';

// disable until we get cli 1.5+ and angular 5+ to be able to run this
Copy link
Member

Choose a reason for hiding this comment

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

Can this comment go now?

aio/content/examples/i18n/src/locale/messages.xlf
Hello i18n!
</source>
<context-group purpose="location">
<context context-type="sourcefile">app\app.component.ts</context>
Copy link
Member

Choose a reason for hiding this comment

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

Is the \ (instead of /) as path separator intentional? Does it make any difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no difference, this is purely for documentation (it's generated and not used)

@gkalpak gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 2, 2017
@gkalpak gkalpak added PR target: TBD and removed target: major This PR is targeted for the next major release labels Nov 2, 2017
@gkalpak
Copy link
Member

gkalpak commented Nov 2, 2017

The Travis failures will probably go away with a rebase on master.
Shouldn't this land on 5.0.x as well. If that is the case, I think you need to label it as target: master and patch.

Foxandxss
Foxandxss previously requested changes Nov 2, 2017
View reviewed changes
aio/tools/examples/example-boilerplate.js Outdated
@@ -31,6 +31,26 @@ const BOILERPLATE_PATHS = {
'tsconfig.json',
'tslint.json'
],
i18n: [
Copy link
Member

Choose a reason for hiding this comment

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

You don't need any of this.

@ocombe ocombe added this to the 5.0.x milestone Nov 2, 2017
@mary-poppins
Copy link

You can preview fcf6586 at https://pr19975-fcf6586.ngbuilds.io/.

@ocombe ocombe added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 2, 2017
@vicb
Copy link
Contributor

vicb commented Nov 2, 2017

@ocombe where to merge ?

@ocombe ocombe added target: patch This PR is targeted for the next patch release and removed PR target: TBD labels Nov 2, 2017
@ocombe
Copy link
Contributor Author

ocombe commented Nov 2, 2017

should be master & patch apparently

@vicb vicb merged commit 132c071 into angular:master Nov 2, 2017
vicb pushed a commit that referenced this pull request Nov 2, 2017
@ocombe @vicb
docs(aio): updated i18n guide and example (#19975)
6f05dab
@ocombe ocombe deleted the feat/aio-i18n-guide-v5 branch November 10, 2017 08:46
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

@Foxandxss Foxandxss Foxandxss left review comments

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
5.0.x
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ocombe @mary-poppins @gkalpak @vicb @Foxandxss @googlebot