search

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

Removing Glyphicons dependency #28

Closed davewalk closed 9 years ago

davewalk commented 9 years ago

This pull request will involve remove this plugin's dependency on the Glyphicons font library for displaying a close button in the notification bar (#18).

For now it just adds an iconClasses attribute to the directive that overrides the default "glyphicon glyphicon-remove close-click" classes if declared.

@alexbeletsky @vikeen if this looks good to you I'll remove glyphicons in other places before merging:

  1. Elements in the CSS file
  2. Add to the docs that you must include your own font library and declare a iconClasses attribute or at least import glyphicons somehow for the default
  3. Remove the fonts/ directory
  4. Remove the Glyphicons SASS file and @import in the main SASS file
  5. Include glyphicon fonts in the example/ directory so that the example app still works correctly with the default

I think that covers everything.

Let me know! Thx!

vikeen commented 9 years ago

Attributes of a directive are really just examples of isolated scope. Is there an issue with using one time binding in the template via isolated scope? It doesn't fundamentally change anything. It's just more of the 'angular' way.

alexbeletsky commented 9 years ago

@davewalk that's a really good start!

My feedback,

  1. I would use closeIcon attribute name, since it's more clear.
  2. As I mentioned in #18 it would be nice to have default implementation, so user just
<notifications-bar class="notifications"></notifications-bar>

If she really want's to override close icon,

<notifications-bar class="notifications" closeIcon="my-custom-icon"></notifications-bar>

Otherwise, it looks great.

davewalk commented 9 years ago

@alexbeletsky I changed iconClasses to closeIcon. Yes, the default is glyphicon glyphicon-remove if the user does not define a closeIcon attribute.

This is ready to go unless you see anything that needs to be changed. I didn't bump up the version # - didn't know if you wanted to do that.

alexbeletsky commented 9 years ago

Amazing job!

I'm merging that in :)

I'll pick up from here and try to intergrate the default close button as embedabble css style. So, it backward compatible and users don't need to do anything after upgrade.

After that I'll publish update.