991856 - [Camera][Madai] Implement the new design for the focus ring

Status: RESOLVED FIXED

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

Description

[Diego Marcos [:dmarcos]]

With the new design the old focus ring looks dated. Even if we still don't have touch to focus we can go ahead and implement the redesigned ring. (see attached the specs for the new focus ring and a first implementation that prashad did that we can use as a start point)

Comment 1

[Diego Marcos [:dmarcos]]

Attachment: Focus ring visual spec

Comment 2

[Diego Marcos [:dmarcos]]

Attachment: Pull request with a first implementation of the focus ring by prashad

Comment 3

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 4

[Amy Lee [:amylee] UX]

Comment on attachment 8402920 [details] [review]
pull-request (master)

Feedback:

- Can you smooth out the transition so there isn’t a jump when the ring turns green? 
- Increase the speed of the rotation so it feels snappy
- When it rotates, can the rotation go past the green focus point and then snap back before it turns green? (simulating the focusing on the lens of a camera)

Thanks!

Comment 5

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8402920 [details] [review]
pull-request (master)

I agree with Amy that it feels a little choppy.

Amy - do you think the focus ring is too thin? It may be a little hard to see for some people and on certain backgrounds. Thoughts?

Comment 6

[Wilson Page [:wilsonpage]]

(In reply to Amy from comment #4)
> Comment on attachment 8402920 [details] [review]
> pull-request (master)
> 
> Feedback:
> 
> - Can you smooth out the transition so there isn’t a jump when the ring
> turns green? 
> - Increase the speed of the rotation so it feels snappy
> - When it rotates, can the rotation go past the green focus point and then
> snap back before it turns green? (simulating the focusing on the lens of a
> camera)

Not 100% clear on what you mean by this, are you able to clarify?

> 
> Thanks!

Did we decided whether or not the focus ring should disappear on 'shutter' or shortly after?

Comment 7

[Amy Lee [:amylee] UX]

Attachment: Focus_Locked_Ring.svg

Increased ring thickness

Comment 8

[Amy Lee [:amylee] UX]

Attachment: Focus_Locking_Ring.svg

Increased ring thickness

Comment 9

[Amy Lee [:amylee] UX]

(In reply to Tiffanie Shakespeare from comment #5)
> Comment on attachment 8402920 [details] [review]
> pull-request (master)
> 
> I agree with Amy that it feels a little choppy.
> 
> Amy - do you think the focus ring is too thin? It may be a little hard to
> see for some people and on certain backgrounds. Thoughts?

Hi Tif, 

I agree and have uploaded thicker rings to replace the current ones.

Comment 10

[Amy Lee [:amylee] UX]

(In reply to Wilson Page [:wilsonpage] from comment #6)
> (In reply to Amy from comment #4)
> > Comment on attachment 8402920 [details] [review]
> > pull-request (master)
> > 
> > Feedback:
> > 
> > - Can you smooth out the transition so there isn’t a jump when the ring
> > turns green? 
> > - Increase the speed of the rotation so it feels snappy
> > - When it rotates, can the rotation go past the green focus point and then
> > snap back before it turns green? (simulating the focusing on the lens of a
> > camera)
> 
> Not 100% clear on what you mean by this, are you able to clarify?
> 
> > 
> > Thanks!
> 
> Did we decided whether or not the focus ring should disappear on 'shutter'
> or shortly after?

Spoke to Wilson so he's clear now. He will be providing revised animation to review.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8402920 [details] [review]
pull-request (master)

Amy/Tif: I have updated the icon font on Wilson's branch using the SVG's provided by Amy. Please re-review Wilson's branch. Thanks!

David: As I mentioned on IRC, I wasn't sure what the policy is here. This is 95% Wilson's code and I had previously given an R+ before he made the suggested UI changes and I then updated the icon font.

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8402920 [details] [review]
pull-request (master)

Looks good to me. Thanks for finishing this up, Justin.

I was worried that it would conflict with my C-AF patch, but that patch got backed out because I forgot to wait for Travis :-(

Comment 13

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8402920 [details] [review]
pull-request (master)

From my side, things look good. I'm not sure the animation Amy was going for so she will have to comment on that bit.

Comment 14

[Amy Lee [:amylee] UX]

Comment on attachment 8402920 [details] [review]
pull-request (master)

Hi, 

The rings look okay now but I had also provided feedback on animation edits that I don't see implemented in this patch (see comment 10). Thanks

Comment 15

[Amy Lee [:amylee] UX]

Attachment: Focus_Locked_Ring.svg

New locked ring

Comment 16

[Amy Lee [:amylee] UX]

Comment on attachment 8402920 [details] [review]
pull-request (master)

This looks GREAT. Thanks Justin!

Comment 17

[Tiffanie Shakespeare [:tif]]

I agree - that focus animation looks really good! Nice job Amy and Justin!

Comment 18

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/5d04a15b284091b6f219d04fac43488fb8081b77

David: Does this need to be 1.4+?

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8402920 [details] [review]
pull-request (master)

This is such a nice visual win that I suggest we uplift to 1.4. It seems low risk to me.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: user will see old, boring viewfinder
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): it is a relatively big patch, but most of the changes are CSS and icon font stuff. There is very little actual JS code here.
[String changes made]: none

Comment 20

[Preeti Raghunath(:Preeti)]

Comment on attachment 8402920 [details] [review]
pull-request (master)

It well beyond the date for approval. But this is a good one to take and given all the work that's gone into the camera work, I'm taking this for 1.4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for v1.4 uplift.

Comment 22

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v1.4)

Pull request for v1.4. Carrying R+ forward.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/6d66329d0c76340485e7b57197264cfd40fdf88c