Notification click behavior

Notification click behavior Don't remove notifications from list on click. Resolution should result in server side changes and then updates which will evict the existing notifications from the visible list. clickoutside behavior to close the menu on outside interaction or when view-all is clicked. Commented out reference to Charm Panel in notifications. Want to talk about this in review. https://code.launchpad.net/~bcsaller/juju-ui/notifications-click-behavior/+merge/127388 (do not edit description out of merge proposal)

[bcsaller, 2012-10-01 23:26:29]

Please take a look.

[hazmat, 2012-10-02 15:23:52]

where'd the tests go? https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js File app/views/notifications.js (left): https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js#oldc... app/views/notifications.js:90: slow_render: function() { s/slow_render/slowRender new to this branch, but since we're here, and the branch is small.. https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js File app/views/notifications.js (right): https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js#newc... app/views/notifications.js:191: closeIndicator: function() { s/closeIndicator/close the former can be mistaken as a noun.

[bac, 2012-10-02 15:43:48]

I merged this branch into trunk and tried running the test but got a lot of failures. I then grabbed a clean copy of the branch and still see lots of failures. Trying 'make clean' and clearing out the browser history didn't help. Have not yet investigated what is going on. https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js File app/views/notifications.js (right): https://codereview.appspot.com/6596054/diff/1/app/views/notifications.js#newc... app/views/notifications.js:199: list.hide(); Why use 'indicator.ancestor' instead of 'parent' here? Oversight or am I missing something?

[bac, 2012-10-02 15:49:43]

Also, I'm confused as to how you got the click anywhere outside of the list to close the notifications. Could you explain that? There was a card for just that task that can be deleted now.

[benjamin.saller, 2012-10-02 20:20:04]

http://yuilibrary.com/yui/docs/api/modules/event-outside.html describes the outside events that are used to make this work. The view-all link is an exception (as it needs the same behavior but is inside the menu) so its bound as well.

[bcsaller, 2012-10-04 09:34:53]

Please take a look.

[gary.poster, 2012-10-04 17:10:51]

Hey Ben. Looks good. I have one request--remove the comment about the bad notifications/charm panel connection and the code as well, as I mention in line--and the rest are just thoughts. If you agree with them, act on them, but if you don't feel free to ignore. https://codereview.appspot.com/6596054/diff/6001/app/views/notifications.js File app/views/notifications.js (right): https://codereview.appspot.com/6596054/diff/6001/app/views/notifications.js#n... app/views/notifications.js:46: //.getInstance().hide(); You are right. I'm sorry that I didn't catch this as a must fix in the review of the branch that added it. I had deferred a number of things to fix later, but this shouldn't have been one of them. Actually, I think a branch in review will make this unnecessary: it does the same trick of hiding the charm popup if you click outside of it. Please just delete this code. https://codereview.appspot.com/6596054/diff/6001/app/views/notifications.js#n... app/views/notifications.js:107: } I'm guessing that's for tests? quick explanatory comment would be helpful. https://codereview.appspot.com/6596054/diff/6001/app/views/notifications.js#n... app/views/notifications.js:200: } for test? https://codereview.appspot.com/6596054/diff/6001/app/views/notifications.js#n... app/views/notifications.js:207: } when would this happen? https://codereview.appspot.com/6596054/diff/6001/test/test_notifications.js File test/test_notifications.js (right): https://codereview.appspot.com/6596054/diff/6001/test/test_notifications.js#n... test/test_notifications.js:58: }; I wonder if we ought to stash this in test utils so we have a reusable bit. It seems like a lot of the tests have a data structure like this now.j

[bcsaller, 2012-10-04 17:36:30]

*** Submitted: Notification click behavior Don't remove notifications from list on click. Resolution should result in server side changes and then updates which will evict the existing notifications from the visible list. clickoutside behavior to close the menu on outside interaction or when view-all is clicked. Commented out reference to Charm Panel in notifications. Want to talk about this in review. R=hazmat, bac, benjamin.saller, gary.poster CC= https://codereview.appspot.com/6596054