Skip to content

Conversation

maxime-allex
Copy link
Contributor

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
See [i18n] enable control over translation message ID

What is the new behavior?
Developer can now use specific ID in translations by adding a new "parameter" in the i18n attribute :

<p i18n="meaning|description@id">foo</p>
<!-- i18n: meaning1|desc1@@id1 -->bar<!-- /i18n -->

will produce for XMB :

<msg id="id" meaning="meaning" description="description">foo</msg>
<msg id="id1" meaning="meaning1" description="description1">foo</msg>

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@maxime-allex
Copy link
Contributor Author

@vicb I created new PR, because I'm not very good at rebasing. Now you have a clean PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think or renaming i18nInfo to msgInfo / msgMeta ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vicb OK for msgMeta.

Copy link
Contributor

Choose a reason for hiding this comment

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

_parseMessageMeta would look better, WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

please return an obj now, would be clerarer

Copy link
Contributor

@vicb vicb Dec 6, 2016

Choose a reason for hiding this comment

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

Please look for @@ first.

  • split left part on | for m|d,
  • right part is id

add const MEANING_SEPARATOR = "|" and const ID_SEPARATOR = "@@"

Copy link
Contributor

Choose a reason for hiding this comment

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

add trailing "," here please - and maybe move id first (this is the important part)

@vicb
Copy link
Contributor

vicb commented Dec 6, 2016

Thanks !

I have added a last round of comments.

To rebase, update your master branch from upstream (or origin whatever it is name) and do a git rebase master -i in your feature branch.
That's pretty easy, do not hesitate to ask if you need help.

Also please remove the WS before the colon in feat(..) : ..., thanks

@vicb vicb added area: i18n Issues related to localization and internationalization action: review The PR is still awaiting reviews from at least one requested reviewer feature Issue that requests a new feature action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 6, 2016
@vicb
Copy link
Contributor

vicb commented Dec 6, 2016

Also we discussed about an issue/PR on the doc repo, any progress on that front ?

@maxime-allex
Copy link
Contributor Author

Copy link
Contributor

Choose a reason for hiding this comment

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

move the const at file level

Copy link
Contributor

@vicb vicb Dec 7, 2016

Choose a reason for hiding this comment

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

simplify, something like:

const idIndex = i18n.indexOf(ID_SEPARATOR);
const [mAndD, id] = idIndex == -1 ?  [i18n, ''] : [i18n.slice(0, idIndex), i18n.slice(idIndex + 2)];
const descIndex = i18n.indexOf('|');
const [meaning, description] = descIndex == -1 ? ['', mAndD] : [mAndD.slice(0, descIndex), i18n.slice(descIndex + 1)];

return {meaning, description, id};

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "id only"

@vicb
Copy link
Contributor

vicb commented Dec 7, 2016

Thanks, this PRs looks good. Only a couple more changes before this can be merged.

@vicb vicb 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 Dec 9, 2016
@vicb
Copy link
Contributor

vicb commented Dec 9, 2016

Thanks! it should get in tomorrow.

@vicb vicb removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 9, 2016
@vicb vicb closed this in 56c361f Dec 9, 2016
@kibbled
Copy link

kibbled commented Dec 21, 2016

Would this feature allow us to set IDs in our HTML/XLF files and avoid having to maintain IDs changing?

@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 10, 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 Issues related to localization and internationalization cla: yes feature Issue that requests a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants