added simple directive #31
Conversation
|
Hello, thanks for this PR, it's very complete and I like that ! I'm a bit worried about the fact that it can use the content of the element because of the risks of injection. |
|
Yeah, you're totally right about nativeElement, but I didn't found out how to adequately manipulate the content from a |
|
Well you're using innerHTML, which means that you don't check the content of the element. In angular 1 we had $sce that did that for us. |
|
There is not much documentation about the right way to manipulate the DOM, so I just took a look how the angular native directives do it. (for example ngClass ) constructor(
public translateService: TranslateService,
public ref: ElementRef,
public renderer:Renderer
) {
}
updateValue(key: string) {
this.translateService.get(key, this.interpolateParams).subscribe((res: string) => {
this.renderer.setText(this.ref.nativeElement, res);
});
} |
|
@hpop: but your code prevents using html in translations. |
|
Is html output necessary? As far as I understand it, there is no html support in pipes, so ng2-translate does't support html output right now. Or am I missing something? |
|
Apart from questioning html support, everything you wrote is correct. The thing is, I'm pretty certain, html support is essential. |
|
Ok. Good point. |
|
If what we need is an Angular2 sanitizer to make this directive a reality, then I could make an Angular2 html sanitizer library based off this excellent one: Try it out here: There's also this alternative: If everyone is in agreement that having one would solve this and then integrating such a library here would allow this PR to be merged, I could create this lib this week. |
|
@NathanWalker That would be great!I had a look into html-sanitizer and unfortunately it has some other dependencies to the caja tool :-/ I hope this is not a showstopper. On the other hand, sanitize-html is browserified and minified still 125K, so maybe resolving those dependencies in cajas html-sanizier would be a good start. |
|
Maybe we could just add a note in the docs saying: "hey be careful" and go on with it ? |
|
Be careful warning may suffice for time being. I think if a sanitizer is created, we could add a wiki here that described how to use the sanitizer with the library if desired. Maybe just take consideration in how this directive is implemented to allow easy plug/play with a 3rd party sanitizer 👍 |
|
Yeah I think that's a better way to do that. I'll try to merge this tonight |
| */ | ||
| updateValue(key: string) { | ||
| this.translateService.get(key, this.interpolateParams).subscribe((res: string) => { | ||
| this.ref.nativeElement.innerHTML = res; |
There was a problem hiding this comment.
I would be weary of this as this would not work in a native environment like NativeScript for instance. May be a way to let the Renderer handle this?
There was a problem hiding this comment.
yeah the PR is old and a lot of things have changed and need to be updated :)
There was a problem hiding this comment.
Maybe allow this to go, and then I could make a {N} plugin that would provide a native alternative to this directive that could be provided for at runtime.
I wouldn't let it hold up a merge.
There was a problem hiding this comment.
If someone could help me with the Renderer approach, I would be really glad :D
|
Now that I've fixed the OnPush bug with the Pipe, do we still need this directive ? |
|
I would really enjoy to see my templates more like |
|
Oh, I totally forgot about the html thing. How can we achieve the rendering of html-translations (e.g. for formatted message bodies) without having the directive? |
|
What are the html translations you're talking about ? |
|
I thought of html translations e.g. for messages with variables. The following could be the translation value: <p>Dear {{username}},</p>
<p>Your order "{{order.name}}" has been shipped and will arrive est. {{order.estArrive}}</p>
<p>Sincerly<br />
Your [cool-company-name] Shipping Team
</p>This could be the view: <message-body translate-params="{order:order, username: username}" translate>
</message-body>If this wouldn't be possible, it would be totally cumbersome to create multiple translation values to compose such a message and furthermore, not every language has the same sentence construction, so the variables are sometimes at a different place in the sentence. Do you have a different solution for this problem? |
|
btw we currently use the below as a workaroud as I didnt want to write a complete directive from scratch. Works perfectly import { Component } from '@angular/core';
@Component({
selector: '[jhi-translate],[jhiTranslate]',
template: '<span [innerHTML]="key | translate:args"></span>',
inputs: ['key:jhi-translate', 'args:translate-values']
})
export class <%=jhiPrefixCapitalized%>Translate {
private key: string;
private args: any;
//FIXME add support to pass translate-compile/ directives in translated content doesnt work
} |
|
@deepu105 I don't think there is a need of prefixing the translate attribute-component with ng. When I created this, I had the compatibility to pascal precht's angular-translate in mind. |
|
@sclausen take a look at this thread angular-translate/angular-translate#849 |
|
@deepu105 I'm not sure, what you want to tell me with this thread. They discuss prefixing because of a very narrow corner case and don't come to a conclusion to implement this, so angular-translate still works like always and if I want to be compatible, I shouldn't implement prefixing. |
|
@sclausen what I'm trying to tell you is that the translate directive doesnt work when you use it in protractor coz protractor treats its as a html5 attribute and expects a boolean and not string as value. So I understand you want to maintain backword compatibility so what say you support both like below So that people who are not bothered by the issue can use |
|
@deepu105 that sounds really reasoned, I will add this. For the rest of my problems though I still have no solution. |
|
@sclausen what do you think of the wrapper directive approach I posted above. It works without any problem for me. But im no ng2 expert so wouldn't know if it has any downfall compared to your original service ? |
|
@deepu105 I don't think your wrapper would work for my desired featureset. The wrapper needs to have |
|
So what's left to be done here? I'll be glad to help if some resources are needed |
|
@gitnik I still have the problem, that |
|
I had a similiar issue. Maybe this can help you?
#288
|
| * Clean any existing subscription to change events | ||
| * @private | ||
| */ | ||
| _dispose(): void { |
There was a problem hiding this comment.
Add the private modifier and remove the _.
private dispose(): void {
| * @param changes | ||
| */ | ||
| ngOnChanges(changes: { [key: string]: SimpleChange; }) { | ||
| if (changes["interpolationParams"] && this.key) { |
There was a problem hiding this comment.
single quotes -> changes['interpolationParams']
| constructor( | ||
| public sanitizer: Sanitizer, | ||
| public translate: TranslateService, | ||
| public _elRef: ElementRef, |
There was a problem hiding this comment.
Why the underscores?
| template: '<ng-content></ng-content>' | ||
| }) | ||
| export class TranslateComponent implements OnInit, OnChanges, OnDestroy { | ||
| @Input('translate') |
There was a problem hiding this comment.
It's bad practice to rename the bindings like this. Why not @Input() translate: string directly?
| @@ -315,4 +315,3 @@ Ionic 2 is still using angular 2 RC4, but ng2-translate uses RC5. You should fix | |||
| ## Additional Framework Support | |||
|
|
|||
| * [NativeScript](https://www.nativescript.org/) via [nativescript-ng2-translate](https://github.com/NathanWalker/nativescript-ng2-translate) | |||
|
|
|||
There was a problem hiding this comment.
No unrelated changes
|
@SamVerschueren thanks for the remarks. I will fix this minor codestyle change requests along with fixing the faulty functionality. I'm currently a little out of spare time. |
|
No problem, we all have busy lives! |
|
I have a question about this, let's say that you have the following html code: <div translate>
Some text
<span>some inner text</span>
</div>then using the innerHTML as a key will receive: Two problems here: the first is that we have the empty lines at the beginning and end which can be removed if you compress your html or use aot, and then it won't work anymore, but you probably won't use that as a key anyway because that'd be insane (so you'll use the <div translate>
Some text
</div>might expect the key to be "Some text", and that will not be the case. And the second problem is that angular adds attributes to inner elements, so that means that you can never use inner html elements in your keys because it won't work. This means that this directive will only work on really simple elements that only contain some text, that's still useful but I would love if it could do more. And to make sure that it works we should trim the content to remove leading/trailing spaces. Do you think we could do something like that: <div translate>
Some text
<span translate>some inner text</span>
</div>And in this case replace only the text at the first level, which would mean that here we have 2 translations: |
|
@ocombe I think your example of as a key is a corner case which shoudln't be handled. Your translation keys are keys in a json file and if someone want's to have something like that as a translation key, I would be curious to hear the explanation why. In your third example, I agree, I would trim al the whitespace, like in angular-translate from @PascalPrecht (translate.js#L167), too. <pre translate="TRANSLATION_ID"></pre>
<pre translate>TRANSLATION_ID</pre>
<pre translate translate-attr-title="TRANSLATION_ID"></pre>
<pre translate="{{translationId}}"></pre>
<pre translate>{{translationId}}</pre>
<pre translate="WITH_VALUES" translate-values="{value: 5}"></pre>
<pre translate translate-values="{value: 5}">WITH_VALUES</pre>
<pre translate="WITH_VALUES" translate-values="{{values}}"></pre>
<pre translate translate-values="{{values}}">WITH_VALUES</pre>
<pre translate translate-attr-title="WITH_VALUES" translate-values="{{values}}"></pre>
<pre translate="WITH_CAMEL_CASE_KEY" translate-value-camel-case-key="Hi"></pre>@SamVerschueren that's also the reason I used A lot has happened since I opened the pull request and much work has to be done to finish this directive/component. Currently my focus in angular2 is on other things, and I think I should rewrite this whole pr, but this may take a while. |
|
No problem, I started rewriting it, using some of your work, but transforming it in a directive instead of a component :) |
|
@ocombe That's great! 👍 |
|
Let's keep this opened, I'll work from your PR so that you get the credits that you deserve, also this will help me not to forget to merge it :D |
|
Done ! |
|
So awesome! Thanks! |
The translation key can be passed as an attribute, or the content of the element. It's also possible to pass an object with interpolation key-value pairs.