angular-notify icon indicating copy to clipboard operation
angular-notify copied to clipboard

Improved placement, individual duration, events and minor refactorings

Open snrlx opened this issue 9 years ago • 5 comments

I added the following functionality:

  • Notifications can now be placed on the left side of the screen
  • Notifications that are placed on the right side are now really on the most right position possible (with a tiny margin of course).
  • It is now possible to call notify() with a duration option, which overwrites the configured duration for the single notification.
  • I updated the example page and added a dropdown to choose the placement of the notification as well as an input field for the duration. Makes it easier for new users to try out stuff.
  • I also made some little code refactorings. E.g. !isUndefined() was used in the code. As angular provides isDefined(), I replaced it. Also there was an unused variable j, which got incremented but never got used.
  • I restricted the input parameters. E.g. position can only be a string.
  • I added 3 events (onOpen, onClick and onClose) for notifications. The functions to call can be supplied as parameters to the notify()-function.
  • I fixed an issue that messages would sometimes remain in the DOM even though their opacity was 0.
  • I updated the README.md accordingly to the newly introduced options.

EDIT: I just saw that I made a mistake when configuring the position. No matter what you use as a parameter, the config remains at the center position. I will fix that as soon as I'm at my local machine :)

I would be happy to get some feedback :)

snrlx avatar Jan 08 '15 10:01 snrlx

Anxiously awaiting a resolution for issue #11. Will this be merged soon?

rich-b avatar Feb 24 '15 19:02 rich-b

Thanks for the PR @snrlx. If you could take a look at the comments please. I will merge soon if so.

Also, if you could squash the commits at the end that would be great.

p.s. I also want to think about the events a little. It seems like there should be a more elegant way to allow the service to send events. Perhaps just triggering some events on $scope, or just having the event listeners on the service itself rather than in the individual notification's config. I would think the use-case would be for listening on all notifications rather than having to set listeners are each one as its created.

cgross avatar Feb 25 '15 00:02 cgross

@cgross : I implemented the CSS classes with 15 pixels for the margins on the left and right side. The CSS classes are now added to the DOM via ng-class as well as ng-style in the case of the center position. This should make sure that everyone who is using an own template is not disturbed by the default styles/classes.

I tried to squash the commits and am not sure if I succeeded to achieve what you wanted. I am kind of new to Git so if that is not the expected outcome, then please tell me what to do :)

snrlx avatar Feb 26 '15 22:02 snrlx

Don't worry about squashing. I can do that when I merge.

On the left/right/center styles, the $centerStyle field does help custom templates provide center styling but the management of the margins for left and right is still a black box that custom templates can't easily override or customize. If you instead use an ng-class by putting the position field on scope and doing the adding/setting of the left/right classes dependent on the value of the position attribute, then a custom template can change that very easily. Basically, try to have zero logic for the positioning in the JS code. Try to put it all in the template. Something like ng-style="{'margin-left': $position === 'center' ? $centeringOffsetMargin : 0}" ng-class="[$classes,$position === 'center' ? 'cg-notify-message-center : '', $position === 'left' ? 'cg-notify-message-left': '', $position === 'right' ? 'cg-notify-message-right': ''] ".

I've thought more about the events as well. I think the elegant solution is to emit events on the message scope rather than having these special onXXX properties of the notification itself. If you use $emit on the notification message scope, then the event will bubble up to the $rootScope. A user can put a listener on $rootScope and then they'd get events for all notifications. If they really only want to listen for events for one individual notification (probably less likely) they can send in a custom scope and put a listener only on that scope (or always keep a listener on $rootScope but just have code in the listener that filters out all but the notifications they care about).

P.S. Working with large pull requests with many features can be difficult. If you'd like to make only a few of these changes you can create different branches in your repo for each separate feature and submit pull requests for each individual feature. That would allow me to merge individual features as they're ready instead of waiting for everything. Up to you though.

cgross avatar Feb 27 '15 02:02 cgross

Could this please be merged soon?

ArekSredzki avatar Oct 16 '15 20:10 ArekSredzki