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

[Angular] Format values of i18n attributes #5875

Closed
voithos opened this issue Feb 15, 2019 · 6 comments · Fixed by #6695 or #7371
Closed

[Angular] Format values of i18n attributes #5875

voithos opened this issue Feb 15, 2019 · 6 comments · Fixed by #6695 or #7371
Labels
lang:angular Issues affecting Angular template (not general JS/TS issues used for Angular) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:enhancement A potential new feature to be added, or an improvement to how we print something

Comments

@voithos
Copy link
Contributor

voithos commented Feb 15, 2019

Prettier 1.16.4
Playground link

--parser angular

Input:

<h1 i18n="This is a very long internationalization description text, exceeding the configured print width, but could easily be formatted">
  Hello!
</h1>
<my-custom-component customAttr="meep" i81n-customAttr="This is a very long attribute-based internationalization description text, which also exceeds the configured print width">
</my-custom-component>

Output:

<h1
  i18n="This is a very long internationalization description text, exceeding the configured print width, but could easily be formatted"
>
  Hello!
</h1>
<my-custom-component
  customAttr="meep"
  i81n-customAttr="This is a very long attribute-based internationalization description text, which also exceeds the configured print width"
>
</my-custom-component>

Expected behavior:
Currently, Prettier doesn't wrap the i18n attribute content. Based on discussions in #5479 and #5482, I think this would be a good candidate for formatting, since the i18n content is well-known, and tends to be long (and thus is more of a pain when unformatted).

Documentation on Angular's i18n attributes, for reference: https://angular.io/guide/i18n

Ideally, I'd want output similar to either this (with a continuation indent):

<h1
  i18n="This is a very long internationalization description text, exceeding the
    configured print width, but could easily be formatted"
>
  Hello!
</h1>
<my-custom-component
  customAttr="meep"
  i81n-customAttr="This is a very long attribute-based internationalization
    description text, which also exceeds the configured print width"
>
</my-custom-component>

Or this (with an alignment to the opening quote, "):

<h1
  i18n="This is a very long internationalization description text, exceeding the
        configured print width, but could easily be formatted"
>
  Hello!
</h1>
<my-custom-component
  customAttr="meep"
  i81n-customAttr="This is a very long attribute-based internationalization
                   description text, which also exceeds the configured print
                   width"
>
</my-custom-component>

I'd somewhat prefer the continuation indent instead of the quote alignment, as it minimizes wasted horizontal space and seems to be more consistent with how Prettier formats long sequences of attributes.

Does this sound reasonable? If so, I may try sending a PR.

@j-f1 j-f1 added type:enhancement A potential new feature to be added, or an improvement to how we print something status:needs discussion Issues needing discussion and a decision to be made before action can be taken lang:angular Issues affecting Angular template (not general JS/TS issues used for Angular) labels Feb 22, 2019
@j-f1
Copy link
Member

j-f1 commented Feb 22, 2019

Not an Angular user, so take this with a grain of salt, but how about this?

<h1
  i18n="
    This is a very long internationalization description text, exceeding the
    configured print width, but could easily be formatted
  "
>
  Hello!
</h1>
<my-custom-component
  customAttr="meep"
  i81n-customAttr="
    This is a very long attribute-based internationalization
    description text, which also exceeds the configured print width
  "
>
</my-custom-component>

@voithos
Copy link
Contributor Author

voithos commented Feb 26, 2019

That should work fine as well - basically, any kind of auto-formatting would be better than the current behavior (which does nothing). Does Prettier already have a precedent of treating attribute values that way? If so, that should be fine - otherwise, I think I'd prefer the first example (with the continuation indent).

@voithos
Copy link
Contributor Author

voithos commented Jun 21, 2019

Just to confirm, does this sound good as-is (i.e. is it ready for a PR) or does it need more discussion?

@j-f1 j-f1 added help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Jun 22, 2019
@j-f1
Copy link
Member

j-f1 commented Jun 22, 2019

Feel free to open a PR!

voithos added a commit to voithos/prettier that referenced this issue Oct 22, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
voithos added a commit to voithos/prettier that referenced this issue Oct 22, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
voithos added a commit to voithos/prettier that referenced this issue Oct 23, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
voithos added a commit to voithos/prettier that referenced this issue Oct 25, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
voithos added a commit to voithos/prettier that referenced this issue Oct 25, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
voithos added a commit to voithos/prettier that referenced this issue Oct 25, 2019
Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes prettier#5875.
thorn0 pushed a commit that referenced this issue Oct 25, 2019
* Add formatting for i18n attributes.

Previously, Prettier would largely ignore i18n attributes, not even
wrapping their content, which wasn't ideal since i18n descriptive text
can sometimes get long.

After this, Prettier will auto-wrap the contents of i18n attributes once
they exceed the line length.

Fixes #5875.
@krilllind
Copy link

Hi, we recently started using prettier in our project and I think that this PR is missing a key point...

If you for example already have a translation for something like:

<div i18n-prop="Special-property|This is a special property with much important information@@MyTextId" prop="My Text"></div>

Then the result translation would be something like:

{
  "MyTextId": "My translated text value"
}

But with this change, running prettier would format my translation to something like:

<div 
  i18n="
    Special-property|This is a special property with much important information@@MyTextId
  "
  prop="My Text"
>
</div>

And my resulting translation id would now be changed to:

{
  "MyTextId\n  ": ""
}

Beasure this is not the same translation id anymore, the xi18n angular cli command will remove my old translation (because there is no reference to it anymore) and insert the new translation id with an empty value.

I think that everything that has to do with translations should be left "as is" to prevent falsy results like this.

Any chance we can change this back to not break the ending " into a new line?

(result could be something like this?)

<div 
  i18n="
    Special-property|This is a special property with much
    important information@@MyTextId"
  prop="My Text"
>
</div>

@thorn0 thorn0 reopened this Jan 16, 2020
@thorn0 thorn0 removed the help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! label Jan 16, 2020
@voithos
Copy link
Contributor Author

voithos commented Jan 20, 2020

Thanks for the fix! I'm wondering, though, if this is a bug in the Angular compiler? Intuitively, I wouldn't expect the @@ to include the whitespace that follows. From what I can tell, the XLIFF spec discourages whitespace in IDs anyway.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 19, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:angular Issues affecting Angular template (not general JS/TS issues used for Angular) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
4 participants