Closed Bug 873574 Opened 8 years ago Closed 7 years ago

[Clock] Clock alarms list needs adjustment

Categories

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

All
Other
defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: epang, Assigned: arasbm)

Details

(Keywords: polish, Whiteboard: visual design, hanzo, visual-tracking)

Attachments

(2 files, 1 obsolete file)

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!
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.
Attachment #761706 - Flags: review?
Attachment #761731 - Flags: review?(pabratowski)
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?
@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.
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!
Attachment #761731 - Attachment is obsolete: true
Attachment #761731 - Flags: review?(pabratowski)
Attachment #763422 - Flags: review?(pabratowski)
(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!
Assignee: pabratowski → arasbm
Flags: needinfo?(arasbm)
Flags: needinfo?(arasbm)
@Eric, here is the link to PR again: https://github.com/mozilla-b2g/gaia/pull/10336/ 
Thank you!
Flags: needinfo?(epang)
Attachment #761706 - Flags: review? → review?(epang)
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.
Attachment #761706 - Attachment mime type: text/plain → text/html
(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!
Flags: needinfo?(epang)
Attachment #761706 - Flags: review?(epang)
Attachment #763422 - Flags: review?(pabratowski)
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 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!
Attachment #761706 - Flags: review?(iliu)
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.
(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 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.
Attachment #761706 - Flags: review?(iliu) → review-
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 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?
Attachment #761706 - Flags: review?(iliu)
Attachment #761706 - Flags: review-
Attachment #761706 - Flags: feedback?(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
Attachment #761706 - Flags: feedback?(oconnore)
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.
(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.
Flags: needinfo?(eric)
With the recent redesign of the clock this bug is no longer relevant.  Thanks you for all the work you put into this Arasbm!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Thanks Eric. No problem, it was a good learning exercise for me anyways :)
Flags: needinfo?(eric)
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.
Attachment #761706 - Flags: review?(iliu)
You need to log in before you can comment on or make changes to this bug.