Improved placement, individual duration, events and minor refactorings #15

snrlx · 2015-01-08T10:41:03Z

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 :)

…ted and fixed right alignment. Minor code fixes.

…ted and right alignment now really puts the notification to the right most point. Minor code fixes.

…ed typechecking.

deborasetton · 2015-01-26T10:23:44Z

angular-notify.js

-                    });
+                switch (args.position){
+                    case 'center':  templateElement.addClass('cg-notify-message-center');
+                                    templateElement.css('margin-left','-' + (templateElement[0].offsetWidth /2) + 'px');


I've found that wrapping this line in a $timeout call (as was being done previously) is actually important. Without that, the "center" case of positioning stopped working for me:

Before:

After:

Thanks, I will take a look into that!

Thanks, and thanks for this PR! I'm using it primarily because of the custom duration feature, but I certainly don't mind getting the other improvements for free as well :)

Ok, fixed that part. Thanks for testing my PR! Please keep me up to date about other insufficiencies :)

rich-b · 2015-02-24T19:59:41Z

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

cgross · 2015-02-24T23:47:25Z

angular-notify.css

+}
+
+.cg-notify-message-left {
+    left:1%;


Can both this and the right class use a px left and right. 10 or15px most likely?

Sure, I will fix that as soon as possible.

cgross · 2015-02-25T00:05:00Z

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.

…ead of percentages

…ted and fixed right alignment. Minor code fixes. Updated Readme added events for opening, closing and clicking a notification. improved typechecking. updated Readme fixed position typechecking, fixed remaining notifications issue fixed positioning by using classes are now read via scope, css positioning done with pixels instead of percentages

snrlx · 2015-02-26T22:14:14Z

@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 :)

cgross · 2015-02-27T02:03:48Z

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.

ArekSredzki · 2015-10-16T20:33:22Z

Could this please be merged soon?

snrlx added 4 commits January 7, 2015 22:56

Each message can have a custom duration. Left alignment is not suppor…

7d41d2f

…ted and fixed right alignment. Minor code fixes.

Each message can have a custom duration. Left alignment is now suppor…

8359649

…ted and right alignment now really puts the notification to the right most point. Minor code fixes.

Merge branch 'master' of https://github.com/snrlx/angular-notify

c97a352

Updated Readme

33ef747

snrlx mentioned this pull request Jan 8, 2015

Support for per-message duration #11

Closed

snrlx added 2 commits January 8, 2015 15:18

added events for opening, closing and clicking a notification. improv…

c31754d

…ed typechecking.

updated Readme

16c79e1

snrlx changed the title ~~Improved placement, individual duration and minor refactorings~~ Improved placement, individual duration, events and minor refactorings Jan 8, 2015

snrlx mentioned this pull request Jan 8, 2015

dispatch some events #7

Closed

fixed position typechecking, fixed remaining notifications issue

e24b6a9

snrlx mentioned this pull request Jan 9, 2015

Messages dont really disappears #10

Closed

deborasetton reviewed Jan 26, 2015
View reviewed changes

fixed positioning by using

96030e0

cgross reviewed Feb 24, 2015
View reviewed changes

snrlx added 3 commits February 26, 2015 17:44

classes are now read via scope, css positioning done with pixels inst…

4e0a4b4

…ead of percentages

Merge branch 'master' of https://github.com/snrlx/angular-notify

9caa66a

snrlx mentioned this pull request Mar 11, 2015

Fix for not disappearing messages #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved placement, individual duration, events and minor refactorings #15

Improved placement, individual duration, events and minor refactorings #15

snrlx commented Jan 8, 2015

deborasetton Jan 26, 2015

snrlx Jan 26, 2015

deborasetton Jan 26, 2015

snrlx Jan 28, 2015

rich-b commented Feb 24, 2015

cgross Feb 24, 2015

snrlx Feb 24, 2015

cgross commented Feb 25, 2015

snrlx commented Feb 26, 2015

cgross commented Feb 27, 2015

ArekSredzki commented Oct 16, 2015

Improved placement, individual duration, events and minor refactorings #15

Are you sure you want to change the base?

Improved placement, individual duration, events and minor refactorings #15

Conversation

snrlx commented Jan 8, 2015

deborasetton Jan 26, 2015

Choose a reason for hiding this comment

Before:

After:

snrlx Jan 26, 2015

Choose a reason for hiding this comment

deborasetton Jan 26, 2015

Choose a reason for hiding this comment

snrlx Jan 28, 2015

Choose a reason for hiding this comment

rich-b commented Feb 24, 2015

cgross Feb 24, 2015

Choose a reason for hiding this comment

snrlx Feb 24, 2015

Choose a reason for hiding this comment

cgross commented Feb 25, 2015

snrlx commented Feb 26, 2015

cgross commented Feb 27, 2015

ArekSredzki commented Oct 16, 2015