Urigo / angular-blaze-template

Include Blaze templates in your angular-meteor application
https://atmospherejs.com/urigo/angular-blaze-template
MIT License
15 stars 6 forks source link

Accept replace attribute to replace the directive element #9

Closed carlosbaraza closed 8 years ago

carlosbaraza commented 8 years ago

This feature would allow people to make ian:accounts-ui-bootstrap-3 work, as the issue is that the parent directive element remains there, eventually breaking the some styling. Some people faced this issue in Urigo/angular-meteor#191 and Urigo/angular-meteor#455.

The usage would be something like: <blaze-template name="_loginButtons" replace></blaze-template>.

Urigo commented 8 years ago

@carlosbaraza this changes the current default behavior, do you think you can change that so that the default behavior will stay the same but you will have option to change that?

carlosbaraza commented 8 years ago

@Urigo, thanks for the check. In effect, I messed up with the previous behavior. Hopefully, it looks better now. I didn't test it though, but it looks good to me.

ozsay commented 8 years ago

This change is a simple version of the replace attribute of directives in angular.

Unfortunately, this attribute is deprecated in angular since it raised a lot of issues. Check out the commit description angular/angular.js@eec6394.

One concern is attaching ng-show/hide/if to the blaze-template element. If the element is replaced, the directives are removed as well.

blaze-template directive can be applied as an attribute. maybe it's a reasonable way to achieve this fix.

carlosbaraza commented 8 years ago

I knew replace was deprecated, but for my concrete use case of using bootstrap 3 login buttons it worked just fine. However I agree replace brings issues with it and that is the reason why it was deprecated. I am not anymore using this as I find it better to implement the directive for the concrete case.

I submitted this as an extra feature that someone could need at some point.

Urigo commented 8 years ago

@ozsay but it looks like the replace here is not using Angular's replace but really removes the whole element and replaces it with the Blaze one. There are no issues with merge attributes as we just remove that. Also it is optional. I think it is still a good option to have

Urigo commented 8 years ago

by the way, @carlosbaraza what other solution did you use?

carlosbaraza commented 8 years ago

What I do, even if it is a bit more work, is creating a package to encapsulate the templates I need to use. Then I create directives reproducing the behavior of this package. This gives me more control over the directives and doesn't limite me, and I get to expose a beautiful and semantic directive :P.

ozsay commented 8 years ago

@Urigo, It's not using the angular replace, but it probably has the same concerns that follow.

One concern is attaching ng-show/hide/if to the blaze-template element. If the element is replaced, the directives are removed as well.

It's a good thing to have, but I think it's worth mention how the replace works or even warn about it.

Also, if we can find a way to accomplish both, we will save a lot of trouble.

@Urigo, @carlosbaraza How can we solve something stupid as this example:

<select ng-model="loginMethod"></select>

<div ng-switch="loginMethod">
  <blaze-template name="facebookLogin" replace ng-switch-when="facebook"></blaze-template>
  <blaze-template name="googleLogin" replace ng-switch-when="google"></blaze-template>
  <blaze-template name="passwordLogin" replace ng-switch-when="password"></blaze-template>
</div>
Urigo commented 8 years ago

@ozsay good point. OK I'm closing

Urigo commented 8 years ago

@carlosbaraza have you tried dotansimha:accounts-ui-angular?

carlosbaraza commented 8 years ago

@ozsay actually those are good points. I can not imagine a good way to accept any directive, without having to implement a handler for each directive separately.

@Urigo, in fact a published a small package to accomplish what I wanted (carlosbaraza:accounts-ui-bootstrap-3-angular). However, it would have the same issue as @ozsay talks about.

In any case, I'd be happy to have this feature, even if I am not able to use it with ng-if or ng-switch. It kind of feels to me that this directive should work like ng-include, which does a replace.

Urigo commented 8 years ago

I mean, we can add that as an option and the responsibility will be on the user, but why would you want that? just so it will be a bit easier with CSS?

Looks like your package is super similar to dotansimha:accounts-ui-angular, what are the differences?

carlosbaraza commented 8 years ago

Well, in my case the issue is that for some reason the styles were broken. I didn't research much the what CSS was broken because I didn't want to patch bootstrap. But probably there is a direct child selector in bootstrap that was breaking the styles.

About my package, it is different only in the fact that it is using bootstrap-3.

Urigo commented 8 years ago

mmm yes and that CSS problem can happen a lot when using existing components.. hmmmm we can add that as an option, the default is not doing that though right?

carlosbaraza commented 8 years ago

Well I tried to leave the default behavior untouched but give the option, in case someone needed it. Though, I think @ozsay has a point in letting know the user that this replace is not compatible with other directives.

Urigo commented 8 years ago

@carlosbaraza so maybe you can add it in the docs in the pull request and I'll merge it?