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

[4.1 only] feat(compiler): support ICU messages in XLIFF #15068

Closed
wants to merge 2 commits into from

Conversation

marclaval
Copy link
Contributor

Fixes #12636

After discussion, we decided to implement the solution 4 from the research document: https://docs.google.com/document/d/1FS02wIBSEbysCoxEqbEmJg3_LyfJ566DZLhhbOUheoA/edit?usp=sharing

return [].concat(...nodes.map(node => node.visit(this)));
}
}

// TODO(vicb): add error management (structure)
// Extract messages as xml nodes from the xliff file
class XliffParser implements ml.Visitor {
private _unitMlNodes: ml.Node[];
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to modify this ?

Is this only to reflect changes in he xmb ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same story as for the XTB loader: translations are first retrieved as string so that ICU messages can be parsed before xml nodes.

@vicb
Copy link
Contributor

vicb commented Mar 10, 2017

LGTM with one question

ocombe
ocombe previously requested changes Mar 13, 2017

visitExpansionCase(icuCase: ml.ExpansionCase, context: any): any {}
ml.visitAll(this, icu.cases).forEach(c => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type on c

@vicb vicb changed the title feat(compiler): support ICU messages in XLIFF [4.1 only] feat(compiler): support ICU messages in XLIFF Mar 20, 2017
@rodush
Copy link

rodush commented Mar 23, 2017

I hope it also fixes the issue of 2.4.8 - when a bit more complex than just basic "select" statement is not extracted into XLIFF file at all...
Example of text:

<ng-container i18n="main-nav.opening-hours|Texts representing opening hours">
    {
        statusText.label, select,
            opening-hours.opened {Nu open (tot {{ statusText.time }})}
            opening-hours.open-tomorrow {Morgen om {{ statusText.time }} zijn we weer open}
            opening-hours.closed {Nu gesloten (we gaan open om {{ statusText.time }})}
            opening-hours.closed-till {Nu gesloten (we gaan maandag open om {{ statusText.time }})}
    }
</ng-container>

where statusText is an object assigned in component, has view e.g.: let statusText = {label: 'opening-hours.opened', time: '18:00'}
Could you guys make sure in your tests that this is extracted properly and translatable.
Thanks!

@vicb vicb added action: merge The PR is ready for merge by the caretaker pr_action: queued labels Mar 23, 2017
@vicb vicb closed this in b8d5f87 Mar 24, 2017
sjtrimble pushed a commit to sjtrimble/angular that referenced this pull request Mar 25, 2017
@ocombe ocombe mentioned this pull request May 2, 2017
20 tasks
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@marclaval marclaval deleted the xliff branch December 11, 2017 09:41
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ICU message for xliff
5 participants