Closed Bug 1092536 Opened 5 years ago Closed 5 years ago

add animation to clearing of all notifications

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: kentaromiura, Mentored)

Details

(Whiteboard: [lang=js])

Attachments

(1 file)

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
Whiteboard: [lang=js]
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)
Comment on attachment 8515483 [details] [review]
add animation to clearing of all notifications

github is playing up, but here's my review:

Good first shot, I really like the effect. There's one major issue, that we can't rely on JS animation like this as it'll perform poorly when the phone is under load. Ideally, we'd use a CSS animation or transition, which would offload the work to the graphics compositor (which has a higher priority than Javascript). I'd like to see an implementation that relies on CSS transitions, if that isn't too much trouble :)

Here are my notes:

Generally: Nice effect, but uses JS animation and missing semi-colons (picked up by linter)

in closeNotification;

- I think we prefer to not initialise multiple variables with commas
- We prefer to '.bind(this)' on the function rather than keep a reference outside of the function to 'this'
- Use a CSS animation or transition
- '120' is a magic number, should be a const with a descriptive name or in the style

in clearAll;

- '80' is a magic number move this into a const at the top of the file (or factor this out into CSS)
Attachment #8515483 - Flags: review?(chrislord.net) → feedback+
Attachment #8515483 - Flags: review?(chrislord.net)
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.
Attachment #8515483 - Flags: review?(chrislord.net)
Attachment #8515483 - Flags: review?(chrislord.net)
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 :)
Assignee: nobody → kentaromiura
Status: NEW → ASSIGNED
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.
Attachment #8515483 - Flags: ui-review?
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.
Attachment #8515483 - Flags: ui-review?
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))
Attachment #8515483 - Flags: ui-review?
Eric, can you take a look at this? We just need a ui sanity check before going in.
Flags: needinfo?(epang)
(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 ;)
Flags: needinfo?(epang)
Awesome, Chris do you want to do the honors of landing?
Flags: needinfo?(chrislord.net)
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!
Flags: needinfo?(kentaromiura)
Merged, and congrats :) https://github.com/mozilla-b2g/gaia/commit/046e2783d2be7b6103a7220aa691fd14f4acb76a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.