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

feat(i18n): add support for custom placeholder names #8010

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@kara
Contributor

kara commented Apr 11, 2016

No description provided.

@@ -93,6 +93,38 @@ export function main() {
.toEqual([new Message('Hi <ph name="0"/> and <ph name="1"/>', null, null)]);
});
it('should replace interpolation with named placeholders if provided (text nodes)', () => {
let res = extractor.extract(`
<div i18n>Hi {{one //i18n(ph="FIRST")}} and {{two //i18n(ph="SECOND")}}</div>`,

This comment has been minimized.

@vsavkin

vsavkin Apr 11, 2016

Contributor

Not sure if we care. Do we want to allow // i18n ( ph = "second" )?

This comment has been minimized.

@kara

kara Apr 11, 2016

Contributor

I suppose we do. Probably would be less confusing for people. I'll update and add a test.

@@ -1003,6 +1003,34 @@ Property binding a not used by any directive on an embedded template ("[ERROR ->
});
describe('comments in expressions', () => {
it('should ignore comments in bound text', () => {
expect(humanizeTplAstSourceSpans(parse('{{a //i18n(ph="NAME"}}', [])))

This comment has been minimized.

@vsavkin

vsavkin Apr 11, 2016

Contributor

It is not really an error, but you are missing a )

This comment has been minimized.

@vsavkin

vsavkin Apr 11, 2016

Contributor

Maybe just change it to comment, so it is consistent with the rest of the tests here.

This comment has been minimized.

@kara

kara Apr 11, 2016

Contributor

Ack, thanks for noticing. I'll switch to comment.

@@ -130,6 +133,24 @@ export function removeInterpolation(value: string, source: ParseSourceSpan,
}
}
export function getPhNameFromBinding(input: string, index: number,

This comment has been minimized.

@vsavkin

vsavkin Apr 11, 2016

Contributor

It is unfortunate that a function called getPhNameFromBinding actually mutates one of its argument. Maybe rename the function or extract the mutation into a separate call.

This comment has been minimized.

@kara

kara Apr 11, 2016

Contributor

I'll separate it back out

@@ -73,7 +74,7 @@ export class Parser {
parseAction(input: string, location: any): ASTWithSource {
this._checkNoInterpolation(input, location);
var tokens = this._lexer.tokenize(input);
var tokens = this._lexer.tokenize(this._stripComments(input));

This comment has been minimized.

@vsavkin

vsavkin Apr 11, 2016

Contributor

Could you add some unit tests for the parser?

This comment has been minimized.

@kara

kara Apr 11, 2016

Contributor

Done

let duplicateNameCount = usedNames.get(name);
if (isPresent(duplicateNameCount)) {
usedNames.set(name, duplicateNameCount + 1);
name = `${name}_${duplicateNameCount}`;

This comment has been minimized.

@vsavkin

vsavkin Apr 12, 2016

Contributor

This is a nitpick, so feel free to ignore it. It's better avoid reassigning parameters. It adds unnecessary mutability. It's harder to follow the flow of the computation. In this case it doesn't matter that much, but it may matter in other cases.

export function dedupePhName(usedNames: Map<string, number>, name: string): string {
  let duplicateNameCount = usedNames.get(name);
  if (isPresent(duplicateNameCount)) {
    usedNames.set(name, duplicateNameCount + 1);
    return `${name}_${duplicateNameCount}`;
  } else {
    usedNames.set(name, 1);
    return name;
  }
}

This comment has been minimized.

@kara

kara Apr 12, 2016

Contributor

Ha! That's how I had it originally. I'll change it.

@kara kara deleted the kara:i18n branch Oct 13, 2018

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