Save user response ($cookieStore)

[vikeen]

Most of this is just vendor code from bower ... sorry about that.

I couldn't find any information on how you want a PR done so feel free to toss back some notes.

• The example page will give a decent test case for the cookieStore implementation, but I think the best thing to do would be to use this as a in depth testing ground. At the same time this can be used to showcase more complex examples of the notifications.

[alexbeletsky]

hi @vikeen - thanks a lot for PR.

1. As you can see, I can't merge PR automatically, please rebase on latest master.
2. I would like to see README update as well. How the interface of module affected? why using cookies, what benefits of it? can I turn it on or off?
3. I think I didn't quite the the idea of user response.. can you please give a use case on that?

Thanks :)

[vikeen]

@alexbeletsky

1. I'll rebase and try again, but these are fairly substantial changes that likely won't merge automatically.
2. Will do.
3. I don't understand this one. Can you delineate that a little more for me please? Are you referring to the example page or the source code functionality?

[alexbeletsky]

@vikeen

I don't understand this one. Can you delineate that a little more for me please? Are you referring to the example page or the source code functionality?

sure! for me, notification is something that user just sees and basically either apply no action at all (it just disappears) or press close button. I didn't quite understand: 1. whats the response in this situation 2. why we need to store it to cookies?

[vikeen]

@alexbeletsky

Fair enough. For example, I was using this module for a restaurant application of mine. I wanted a method to show an error notification when the user didn't have geolocation available or they failed to provide access to it (allow / block). In this situation I felt it was beneficial to store their dismissal of the notification to a cookie. This cookie would then be checked when they reload the page or come back to the website at a later date so they don't keep receiving the same notification over and over again. I view it as an optional UX improvement.

Does that help clear it up?

[vikeen]

I believe everything is in order now. In hindsight I probably should have done a pull request into a branch instead of the master branch. That would facilitate a better staging / test ground for the changes.

• The README and example documents have been updated. However, I feel the README document has duplication that could be consolidated near the end with the code examples. Just a small nit pick of mine.

[alexbeletsky]

@vikeen It's much more clear now.

But, according to your description I have some points to mention,

1. Ideally module should have as less dependencies as possible.
2. The responsibility of showing/not-showing notification have to be outside of ng-notifications-bar itself.
3. As a solution I see show methods are actually returning promise which is resolved then user confirms (closes) the notification. In the handler of this promise, the application is storing the info and checks it on next right.

What I'm saying, the use case you described have to solved on a level of application, not the module. And I don't see much of benefit to have that built-in.

What do you think?

[vikeen]

@alexbeletsky

I think that's a fair assessment. I was sort of comparing it to the HTML binding patch that was applied to the module. That is different because without the support of the module you can't receive that functionality. For preventing duplicate notifications to the user this could be solved on two fronts; application or module. In the end it's not really my decision. The code is in the pull request and your more than welcome to use it, but it contributes to a large chunk of code when it's something that could just easily be at the application and only when needed.

I'll let you and others make the call.

[alexbeletsky]

@vikeen

As HTML binding patch, it's indeed need to be the part of module, since we want to have seamless HTML support for notifications. So, I think I'll not put that in, just because it's could be (and should be) solved on application level.

But, I think what would be nice to have is promises I mentioned above, to make the implementation of that feature easy.

I'll close that PR, but going to open new issue to cover that.