Closed Bug 1092536 Opened 6 years ago Closed 6 years ago
add animation to clearing of all notifications
Flame, nightly channel, Nov 1 STR: 1. take a few screenshots 2. open notification tray, clear all notifications Expected: there's some nice animation and fade out Actual: they jarringly blink out of existence
proposed solution: https://github.com/mozilla-b2g/gaia/pull/25716
I implement a simple slide to right animation, I changed the ns_closeNotification in a backward compatible way, so that when no delay are passed it will just work as before (if you try to swipe for example the new effect doesn't collide). On the other hand you can now pass a delay and a waitBeforeClosing parameter, and if you do, there will be a percentual TranslateX applied to each notification. The waitBeforeClosing is needed to avoid the animation that will trigger when a single notification get removed. Another solution would have been to start from the bottom, but that's just does not feel natural
Attachment #8515483 - Flags: review?(chrislord.net) → feedback+
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications I've added my review comments to github - nice work, it's so close now :) I think the finished patch would be low risk enough that we could consider uplifting this... I'll put in the request when it's done.
I've added some comments to the github - it's all just small naming things now, so this is basically an r+ - I'm leaving off the r+ just to make sure this doesn't accidentally get checked in before those changes are made. I've sorted out my github notifications, so I'm leaving the r?me here and I'll + it once you're done :)
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications An r+, but we need to fix a broken test before we can commit it.
Attachment #8515483 - Flags: review?(chrislord.net) → review+
Please make sure to get a ui-review sanity check too before landing.
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications This is ready for UI review. Previous behaviour: Pressing 'Clear all' causes all notifications to disappear instantly, with no animation. New behaviour: Pressing 'Clear all' causes the animations to animate away towards the right, with a small delay between each to give a staggered effect. Functionally, the behaviour is the same, but the animation makes it feel good and less broken.
please wait as I found a bug I introduced when I rewrote it using the css animation as I'm wrongly clearing all the notification, it's a very easy fix, but now I've the same check 3 times, so I'll rewriting it so that I'll prefill an array of clearable notifications first, as in I've already wrote it and I'm testing it right now
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications Removing ui-review re: last comment.
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications Checked it locally, this is ready for UI review now. I think there are bits we could tweak, but this is definitely more than good enough to go in (and is infinitely better than what we have currently (which is nothing))
Eric, can you take a look at this? We just need a ui sanity check before going in.
(In reply to Michael Henretty [:mhenretty] from comment #12) > Eric, can you take a look at this? We just need a ui sanity check before > going in. Thanks for flagging me on this Michael! This looks great, not something I thought we would have time to get in! R+ from me even though I'm not flagged ;)
Awesome, Chris do you want to do the honors of landing?
The three commits I think should be rolled into one before we merge (at least, they should follow the same commit log structure as all our other commits), and although it's not necessary, it'd be preferable to address the two suggestions I've made (1. possible early return, 2. add a comment). I can do these myself, but it'd be nice to be able to merge from your branch so you can track the credit more fully :)
Flags: needinfo?(chrislord.net) → needinfo?(kentaromiura)
Comment on attachment 8515483 [details] [review] add animation to clearing of all notifications :epang gave ui-review+.
Attachment #8515483 - Flags: ui-review? → ui-review+
I'm on it!
Merged, and congrats :) https://github.com/mozilla-b2g/gaia/commit/046e2783d2be7b6103a7220aa691fd14f4acb76a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.