-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Possible Enhancements #15
Comments
@vikeen that's just amazing feedback. I like each of propositions you have and would like to implement it for I've created tickets for each of propositions, this one is about to be closed. Let's move the discussion and actual PR's there. calling @dmaslov, @tommaitland, @jguffey - your help is needed, please join here. |
Awesome! I'm glad I could help provide some feedback. |
Happy to pick up some of these, I made similar changes to my implementation of it. I'm away until March but I'll have a look at what's remaining then. On Sat, Feb 7, 2015 at 12:03 AM, John Rake [email protected]
|
Before continuing before I want to say this was far and away the best notification module I found during my research. Given that I needed to modify it quite substantially I wanted to also praise the code quality because it took barely any time to grasp everything.
Unfortunately, I did run into issues and I would like to share my cases for possible enhancements to the main code base.
getSaveResponse
,setSaveResponse
)saveResponse
also has aid
parameter. This was needed to find correspond correct notifications for dismissal.notification
class to each notification div in the template along with thenote.type
..notification
to target each entire notification. I could have used a child selector, but it just seemed like overkill for a html element that was a notification.Let me know what your thoughts are. The template things are fairly opinionated for myself, but I do believe they are all good features.
The text was updated successfully, but these errors were encountered: