alexbeletsky / ng-notifications-bar

Angular.js component for stylish and flexible top bar notifications.
http://beletsky.net/ng-notifications-bar
149 stars 52 forks source link

Decouple glyphicons #18

Closed alexbeletsky closed 9 years ago

alexbeletsky commented 9 years ago

Decouple glyphicons. Fairly straight forward. I was using font awesome and needed to change the template because of this. The close icon doesn't need to be library specific. Maybe add a way to customize it?

Sub-task of #15.

davewalk commented 9 years ago

I need to do this too and it's easy to do when maintaining your own fork, but I would be willing to send a pull request to improve this for everyone.

Looking at the code, I see two ways in which this could be done:

  1. Adding something like a iconClasses attribute on the directive that injects those classes in the template for the close button <span> element.
  2. Use ngTransclude to allow the user to specify the whole <span> element from the close button. I suppose the benefit here is that the user can opt to not have a close button (or do something else instead of a close button), however they'd have to remember to call the close() on click.

Which do you think is best?

And does this issue include stripping the glyphicon fonts and related CSS, relying on the user to include them if they choose to need them?I personally like this because it gives the user more control over styling of the bar.

Overall, great plugin! Thx!

vikeen commented 9 years ago

Personally, I felt like you should be able to pass in an entire template file. That is sort of what transclude would do, but I'm fairly certain transclude is gone in 2.0.x so I wouldn't advise with that solution.

alexbeletsky commented 9 years ago

@davewalk @vikeen I like the simplicity of solution 1. Franky, I never used ngTrasclude not sure how exactly it is used for this case.

Let's have a decision, based on only one criterion - the simplicity of usage. Ideally, you just need to install library from npm or bower, hook-up .js and .css file and that's it. No fonts folder should be required, as it's now.

What I was thinking. Since it's only close-icon is currently used in plugin, would it be possible to use cross-sign say from font awesome and make embeddable CSS for it? If a user really wants to override the default style, he should be able to provide it's own.

What's your thoughts?

davewalk commented 9 years ago

@alexbeletsky I'll send a pull request soon!

alexbeletsky commented 9 years ago

@davewalk I just thought a bit more about glyphicons issue. now, with your appreciated PR, we thrown glyphicons out of the package. But the user still needs to download fonts (and glyphicons are no longer distributed free) or override it with font-awesome. In both cases, the integration requires an effort, but I would like to make it as smooth as possible.

Because, it's just a close button, simple cross.. why we need to use a font for that in the default implementation. There are few approaches to implement it purely in CSS.

What do you think?

davewalk commented 9 years ago

@alexbeletsky While you can implement a close button in pure CSS, users may already have a font library in their app and will want to be able to include it to keep their style consistent. In fact, this is my use case: I already have a Font Awesome close button in my app so how the directive is set up now works for me.

I brought up the possibility of using ngTransclude earlier to give the user further control on how they want the message to be displayed. It would require the user to implement a template within the directive, but it would give them greater flexibility between using a font library, using pure CSS or some other solution. I'd lean towards this solution over including the close button as CSS only. The more agnostic the module is on presentation the better, in my opinion.

davewalk commented 9 years ago

@alexbeletsky Where are you thinking about going with this? Will my PR be included in the NPM version?

alexbeletsky commented 9 years ago

@davewalk sorry for the delay! package just re-published.. :+1: