873574 - [Clock] Clock alarms list needs adjustment

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia::Clock, defect, P1)

Description

[Eric Pang [:epang] UX]

The layout of the alarm list needs adjustment so the user can tell there are more alarms than just 4.

Przemek, let me know when we are good to move forward with this bug. Thanks!

Comment 1

[Aras Balali Moghaddam]

Attachment: link to pull request on github

This is my first pull request for gaia, I am also new to using bugzilla. Any advice would be highly appreciated. The patch makes some visual changes to the list, so UX and visual review also needed.

Comment 2

[Aras Balali Moghaddam]

Attachment: image showing 3 different visual states for the scroll indicators

Comment 3

[Przemek Abratowski [:przemek] UX]

Comment on attachment 761731 [details]
image showing 3 different visual states for the scroll indicators

I feel the fade should not be white on a black screen. Can the fade be made black?

Comment 4

[Aras Balali Moghaddam]

@Przemek, absolutely. I can change the color to anything you like. I think I may be missunderstanding your comment, I dont think a black gradient would be visible on black background. Do you have a particular shade of dark grey in mind? If you give me the color you want to try, I can update the pull request.

Comment 5

[Aras Balali Moghaddam]

Attachment: image showing alarms list different states after changing color of gradient to black

Przemek, after thinking a bit about your suggestion, I think I finally understood what you meant by changing gradient to black. I made it black and larger so it gives a fading away impression on the side that there is more content to scroll. You can see in the image the 3 states on top have at least one side "fading out" to indicate that there are more alarms to scroll to. When there is 4 or less alarms, there will be no visual difference, as shown in the bottom two image. Let me know what you think. I will add this code to my PR right away so you can take alook at the code as well if you'd like. Thanks!

Comment 6

[Eric Pang [:epang] UX]

(In reply to arasbm from comment #5)
> Created attachment 763422 [details]
> image showing alarms list different states after changing color of gradient
> to black
> 
> Przemek, after thinking a bit about your suggestion, I think I finally
> understood what you meant by changing gradient to black. I made it black and
> larger so it gives a fading away impression on the side that there is more
> content to scroll. You can see in the image the 3 states on top have at
> least one side "fading out" to indicate that there are more alarms to scroll
> to. When there is 4 or less alarms, there will be no visual difference, as
> shown in the bottom two image. Let me know what you think. I will add this
> code to my PR right away so you can take alook at the code as well if you'd
> like. Thanks!

Hi, Przemek is away this week, but this looks good, can you flag me for 'feedback' when you add this to your PR?  I would like to test it on the device.  If it looks good then we'll flag for code review. If not, we can work on adjustments. Thanks!

Comment 7

[Aras Balali Moghaddam]

@Eric, here is the link to PR again: https://github.com/mozilla-b2g/gaia/pull/10336/ 
Thank you!

Comment 8

[Aras Balali Moghaddam]

Comment on attachment 761706 [details]
link to pull request on github

I just updated the pull request to squash all changes into one commit. looking forward to your comments.

Comment 9

[Eric Pang [:epang] UX]

(In reply to arasbm from comment #8)
> Comment on attachment 761706 [details]
> link to pull request on github
> 
> I just updated the pull request to squash all changes into one commit.
> looking forward to your comments.

Hi, sorry for the delay.  This is looking really good!  Can you make one small change?

When scrolling to show the first or last alarm they seems to go from behind the gradient to full opacity too quickly.  All the alarms in between have a smooth and gradually fade in/fade out (which looks correct).  To reproduce this add around 5-6 alarms and slow scroll through the alarms from top to bottom (while focusing on the first and last).  Let me know if I need to clarify! Flag me for feedback when ready, we can save the review flag for code review, which I can request once the visuals look good.

Thanks!

Comment 10

[Aras Balali Moghaddam]

Hi Eric, thanks for your feedback. If understand correctly, adding a short (<300ms) transition effect to the gradients would solve the problem you describe. Actually, I had that transition added, but I removed it after testing since it was not working as expected. I will dig into it more to see how I can add CSS transition to psuedo elements and then will update you. Cheers.

Comment 11

[James Burke [:jrburke]]

Comment on attachment 761706 [details]
link to pull request on github

Asking Ian for a review, as he has been reviewing the other more recent clock changes, and the community member that did the patch has updated it since the last feedback cycle. 

Would be nice to get this community contribution landed, particularly since the pull request includes some nice pictures of the change!

Comment 12

[Aras Balali Moghaddam]

Thank you James for following up on this issue. I updated the PR since one of the files I had modified has been renamed. 

I tried several different approaches to address the valid issue Eric raised above. If I understand correctly we want the indicators to show up and hide gently, as opposed to sudden change in visibility. However, I was having some issues with using CSS animations in this context. My understanding is that simple CSS transitions should work here, but in practice they dont. I put more details as comment in the PR. Any comment/suggestions will be appreciated.

Comment 13

[Eric Pang [:epang] UX]

(In reply to arasbm from comment #12)
> Thank you James for following up on this issue. I updated the PR since one
> of the files I had modified has been renamed. 
> 
> I tried several different approaches to address the valid issue Eric raised
> above. If I understand correctly we want the indicators to show up and hide
> gently, as opposed to sudden change in visibility. However, I was having
> some issues with using CSS animations in this context. My understanding is
> that simple CSS transitions should work here, but in practice they dont. I
> put more details as comment in the PR. Any comment/suggestions will be
> appreciated.

Hi, great work on this!  From a visual stand point the transition looks much smoother. I think it looks good :).  Thanks for your work on this!

Comment 14

[[e-mail], [e-mail]]

Comment on attachment 761706 [details]
link to pull request on github

Hi arasbm,

The improving UX is pretty cool for me. A user is able to know what boundary of the index is located via the gradient. But I'm wondering the lower frame rate while alarm list is scrolling. Especially, alarm list is scrolled to the top and bottom. We could see jitter frame while applying CSS style for alarm list.

My suggestion is as following:

* Have some criteria to decrease the computing time ''showHideScrollIndicators()'' while receive scroll event
* Apply CSS style while alarm list scroll completely

Maybe some other approach that I did not think. Feel free to discuss with us. Thanks you so much for the improving patch.

Comment 15

[Aras Balali Moghaddam]

Ok, I just managed to flash B2G 1.2 on my keon. I will experiment a bit with ways to make this faster and get back to you soon. Thank you for reviewing my code.

Comment 16

[Aras Balali Moghaddam]

Comment on attachment 761706 [details]
link to pull request on github

Ian Liu, could you take a look at the updated patch. It performs much better now.

O'Connore, I was told in IRC I would need feedback from your team about this patch, since apparently alarm clock has been changing significantly since I submit my patch. Would this still be useful?

Comment 17

[Eric O'Connor [:oconnore]]

Comment on attachment 761706 [details]
link to pull request on github

Hi arasbm,

I loaded this on my device today and played around with it. I like the indicators, but am wondering if we could eliminate the 0<->1 opacity jump in the gradients? 

Do you have any ideas on this? 

PS. I rebased onto master: 
https://gist.github.com/oconnore/6393140

Comment 18

[Aras Balali Moghaddam]

Hi Eric,
Thanks for trying out the patch and your feedback. I had a version of this patch before with transition animation for the opacity. However, when testing on Keon, I noticed the animation does not work (most likely gets dropped out because of performance issues). The gradients are positioned carefully so they do not appear/disapear abruptly. To be honest I spend many hours fiddling with this and trying different solutions on Keon. It seems that currently because of performance issues I can not get the tranision animation working in this scenario. If someone can suggest another method, I would be happy to try it out. To be honest though, this is a very small feature and if there are so much performance concerns for very little gain, maybe it is better to drop this idea for now?

Please let me know what you think.

Comment 19

[Eric Pang [:epang] UX]

(In reply to arasbm from comment #18)
> Hi Eric,
> Thanks for trying out the patch and your feedback. I had a version of this
> patch before with transition animation for the opacity. However, when
> testing on Keon, I noticed the animation does not work (most likely gets
> dropped out because of performance issues). The gradients are positioned
> carefully so they do not appear/disapear abruptly. To be honest I spend many
> hours fiddling with this and trying different solutions on Keon. It seems
> that currently because of performance issues I can not get the tranision
> animation working in this scenario. If someone can suggest another method, I
> would be happy to try it out. To be honest though, this is a very small
> feature and if there are so much performance concerns for very little gain,
> maybe it is better to drop this idea for now?
> 
> Please let me know what you think.

Flagging Eric for info so he sees this.

Comment 20

[Eric Pang [:epang] UX]

With the recent redesign of the clock this bug is no longer relevant.  Thanks you for all the work you put into this Arasbm!

Comment 21

[Aras Balali Moghaddam]

Thanks Eric. No problem, it was a good learning exercise for me anyways :)

Comment 22

[[e-mail], [e-mail]]

Comment on attachment 761706 [details]
link to pull request on github

Leave my reviewing flag here since the bug is closed invalid. And leave some comment on Github.