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

RESOLVED FIXED in 1.4 S6 (25apr)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dmarcos, Assigned: justindarc)

Tracking

unspecified
1.4 S6 (25apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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)
Blocks: 983405
Assignee: nobody → wilsonpage
Attachment #8402920 - Flags: ui-review?(tshakespeare)
Attachment #8402920 - Flags: ui-review?(amlee)
Attachment #8402920 - Flags: review?(jdarcangelo)
Attachment #8402920 - Flags: review?(jdarcangelo) → review+
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!
Attachment #8402920 - Flags: ui-review?(amlee) → ui-review-
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?
Attachment #8402920 - Flags: ui-review?(tshakespeare) → ui-review-
Flags: needinfo?(amlee)
(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?
Posted image Focus_Locked_Ring.svg (obsolete) —
Increased ring thickness
Flags: needinfo?(amlee)
Increased ring thickness
(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.
(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.
Assignee: wilsonpage → dmarcos
Assignee: dmarcos → jdarcangelo
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.
Attachment #8402920 - Flags: ui-review?(tshakespeare)
Attachment #8402920 - Flags: ui-review?(amlee)
Attachment #8402920 - Flags: ui-review-
Attachment #8402920 - Flags: review?(dflanagan)
Attachment #8402920 - Flags: review+
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 :-(
Attachment #8402920 - Flags: review?(dflanagan) → review+
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.
Attachment #8402920 - Flags: ui-review?(tshakespeare) → ui-review+
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
Attachment #8402920 - Flags: ui-review?(amlee) → ui-review-
New locked ring
Attachment #8404016 - Attachment is obsolete: true
Comment on attachment 8402920 [details] [review]
pull-request (master)

This looks GREAT. Thanks Justin!
Attachment #8402920 - Flags: ui-review- → ui-review+
I agree - that focus animation looks really good! Nice job Amy and Justin!
Landed on master:

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

David: Does this need to be 1.4+?
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(dflanagan)
Resolution: --- → FIXED
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
Attachment #8402920 - Flags: approval-gaia-v1.4?
Flags: needinfo?(dflanagan)
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
Attachment #8402920 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
Needs a branch patch for v1.4 uplift.
Flags: needinfo?(jdarcangelo)
Target Milestone: --- → 1.4 S6 (25apr)
Posted file pull-request (v1.4)
Pull request for v1.4. Carrying R+ forward.
Attachment #8409692 - Flags: review+
Flags: needinfo?(jdarcangelo)
You need to log in before you can comment on or make changes to this bug.