Closed
Bug 999939
Opened 10 years ago
Closed 10 years ago
[MADAI][CAMERA] No effect on camera option menu back button
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Firefox OS Graveyard
Gaia::Camera
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S3 (6june)
People
(Reporter: singhashish1887, Assigned: singhashish1887)
References
Details
Attachments
(4 files, 2 obsolete files)
64.71 KB,
image/png
|
amylee
:
ui-review-
tif
:
ui-review-
|
Details |
46 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
amylee
:
ui-review+
ver4ffos
:
ui-review?
tif
:
ui-review+
|
Details | Review |
180.60 KB,
image/png
|
Details | |
136.22 KB,
image/png
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140415030203 Steps to reproduce: 1. Camera - Option Menu - go to second menu 2. Click back button Actual results: No effect on back button Expected results: There is buuton click effect(Active effect - blue color) like settings app menu
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8410778 -
Flags: review?(wilsonpage)
As discussed with Ashish, assigning this issue to him.
Assignee: nobody → singhashish1887
Comment 3•10 years ago
|
||
This doesn't look right to me. Flagging amlee and tiff for ui-review.
Attachment #8411000 -
Flags: ui-review?(tshakespeare)
Attachment #8411000 -
Flags: ui-review?(amlee)
Comment 4•10 years ago
|
||
Comment on attachment 8410778 [details] [review] Pull Request The code looks fine. But flagging amlee and tiff for ui-review to make sure this is actually to spec.
Attachment #8410778 -
Flags: review?(wilsonpage) → review+
Comment 5•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #3) > Created attachment 8411000 [details] > screenshot > > This doesn't look right to me. Flagging amlee and tiff for ui-review. Hi Wilson, Can you match the highlight state of the back button in the settings sub menu to the back button highlight state we are using for image preview (white elephant)? Thanks!
Comment 6•10 years ago
|
||
Comment on attachment 8411000 [details]
screenshot
Highlight is not to spec.
Can you match the highlight state of the back button in the settings sub menu to the back button highlight state we are using for image preview (white elephant)? Thanks!
Attachment #8411000 -
Flags: ui-review?(amlee) → ui-review-
Comment 7•10 years ago
|
||
Comment on attachment 8411000 [details]
screenshot
Definitely doesn't look right.
Attachment #8411000 -
Flags: ui-review?(tshakespeare) → ui-review-
Assignee | ||
Comment 8•10 years ago
|
||
Amy and Tiffanie, Please provide the spec for the given scenario.
Flags: needinfo?(tshakespeare)
Flags: needinfo?(amlee)
Comment 9•10 years ago
|
||
Highlight state is visual design and in Amy's realm. I believe she's working on something for you now.
Flags: needinfo?(tshakespeare)
Comment 10•10 years ago
|
||
(In reply to singhashish1887 from comment #8) > Amy and Tiffanie, > Please provide the spec for the given scenario. Hi The highlight state should be the back arrow turning blue (same blue we are using for the camera press states). There should be a brief delay before it turns back to white on release. Once I get the exact delay time I will post it to this bug. Thanks!
Flags: needinfo?(amlee)
Assignee | ||
Comment 11•10 years ago
|
||
Wilson, In camera sub-menu back click in enable on back arrow as well as title. what should be the correct behavior shall we take click on back arrow only or on title also? If we will back on click on arrow only than it will be same as settings menu (highlight arrow). But if we have to back on click of title also than what should be the behavior, only arrow or arrow + title should be highlighted ?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(wilsonpage)
Comment 12•10 years ago
|
||
(In reply to singhashish1887 from comment #11) > Wilson, > In camera sub-menu back click in enable on back arrow as well as title. > what should be the correct behavior shall we take click on back arrow only > or on title also? > If we will back on click on arrow only than it will be same as settings menu > (highlight arrow). > But if we have to back on click of title also than > what should be the behavior, only arrow or arrow + title should be > highlighted ? Hi, I will be provided a spec very shortly to answer these questions. Thanks!
Comment 14•10 years ago
|
||
Hi, Attached is the spec for the back arrow highlight state and high area. Also, I would like to add a 0.3 second delay on the highlight state so when the user releases the button, you still see the blue highlight state for a brief moment before it changes back to white. Let me know if you have any questions. Thanks!
Flags: needinfo?(singhashish1887)
Assignee | ||
Comment 15•10 years ago
|
||
Thanks Amy I am checking the Spec and change the implementation according to Spec.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(singhashish1887)
Comment 17•10 years ago
|
||
Attachment #8417844 -
Flags: ui-review?(amlee)
Attachment #8417844 -
Flags: review?(wilsonpage)
Comment 18•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request - You've added new styling but haven't removed the old styling. - New name=back attribute seems unused. All visual related sign-off left for amylee. Previously the entire header title was the hit target for the back action. Was there an issue with this? I don't feel strongly about this, but seems like it's now more difficult to go back, as the user has to stretch to reach the tricky top-left screen area.
Attachment #8417844 -
Flags: review?(wilsonpage) → review-
Comment 19•10 years ago
|
||
Agreed with Wilson, the entire header has to be clickable.
Comment 20•10 years ago
|
||
Diego and Wilson, I have implemented it according to UI spec given by Amy in comment #14. Let's wait for Amy's comments.
Flags: needinfo?(amlee)
Updated•10 years ago
|
Attachment #8417844 -
Flags: ui-review?(tshakespeare)
Flags: needinfo?(amlee)
Comment 21•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request I'm giving this a ui-review + because it is to spec. To address some of the other comments - there was concern about the size of the menu's tap area being inconsistent with the preview header's tap area. Therefore, we decreased the menu's tap area to be more consistent with the preview header because in this release we are not able to change the header building block nor can we deviate from the building block. We can increase the size of the menu's tap area to make it an easier target, but this will cause the inconsistency again because we cannot change the header building block. If everyone can be onboard with this, Amy can post a revision. However, the tap area should not be the full width of the menu (minus the menu icon). We would extend it to about the end of "Self-timer" to be more forgiving. Let us know if you want to leave the patch as is or create a slightly larger area while leaving the BB alone. Thanks!
Attachment #8417844 -
Flags: ui-review?(tshakespeare) → ui-review+
Comment 22•10 years ago
|
||
Is it final UI or we should wait for further discussion ?
Flags: needinfo?(tshakespeare)
Comment 23•10 years ago
|
||
Hi, Attached is the revised spec for the hit state area based on Tiff's comment. The increase hit area space would make going back to the previous screen easier on smaller screens. Let me know if you have any questions.
Attachment #8413911 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
(In reply to ver4ffos from comment #22) > Is it final UI or we should wait for further discussion ? I've talked to Tiff and this is final UI. Thanks!
Flags: needinfo?(tshakespeare)
Comment 25•10 years ago
|
||
amylee: If you reduce the hit-target to only the arrow, it's almost impossible for the user to see that the arrow has turned blue as it's under their thumb. On the topic of hit targets, I gave my girlfriend the camera to play with. She seems to navigate the UI fine, except the back button. When in the preview-gallery she really struggled to go back. I observed her tapping the text over and over. I had to intervene and tell her to tap the arrow. I understand that this is baked into the header building block, but let's not impose bad UX just stay inline with an existing (broken) pattern. Also worth noting that this patch regresses the work I did on aligning the 'Options' and sub-menu title text (can't remember bug#). You can see that when switching in and out of sub-menus, the title text shifts position (amylee will know what I'm on about).
Flags: needinfo?(tshakespeare)
Flags: needinfo?(amlee)
Comment 26•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #25) > amylee: If you reduce the hit-target to only the arrow, it's almost > impossible for the user to see that the arrow has turned blue as it's under > their thumb. > > On the topic of hit targets, I gave my girlfriend the camera to play with. > She seems to navigate the UI fine, except the back button. When in the > preview-gallery she really struggled to go back. I observed her tapping the > text over and over. I had to intervene and tell her to tap the arrow. > > I understand that this is baked into the header building block, but let's > not impose bad UX just stay inline with an existing (broken) pattern. > > Also worth noting that this patch regresses the work I did on aligning the > 'Options' and sub-menu title text (can't remember bug#). You can see that > when switching in and out of sub-menus, the title text shifts position > (amylee will know what I'm on about). Hi Wilson, Please see comment 23. The hit area has been increased to the length of the header to make it easier to go back to the previous screen. And yes thanks for pointing out the alignment fixes of the headers and sub-headers. I can't seem to find the bug for that. Wilson do you have the patch for that fixed the header alignment? Let's make sure we aren't regressing when we fix this bug.
Flags: needinfo?(wilsonpage)
Flags: needinfo?(ver4ffos)
Flags: needinfo?(amlee)
Comment 27•10 years ago
|
||
What about increasing the target area for the building block as well? It shouldn't be that big of a change and it solves a huge UX flaw. Question for UX: Is anybody working on that? Do we have any ETA? Wilson, if nobody is currently looking at it why don't you submit a patch for the building block?. The gaia headers we have now are terrible and the fix trivial. I feel there's over thinking and paralysis because of fear of regressions. We have to do better.
Comment 28•10 years ago
|
||
dmarcos: There is work being done to improve the building-block. The header text will be centered instead of left aligned and the hit are of the back button will roughly double in size. I think this is fair as the header text does not represent the name of the view you are going back to but instead the current view. I believe mhenretty is currently working on this.
Flags: needinfo?(wilsonpage)
Comment 29•10 years ago
|
||
amylee: I'm not sure what the bug number is for that other issue. The structure of the header has to be changed to implement the reduced hit area requirements, for this reason previous style work on the header will not carry across by default.
Comment 30•10 years ago
|
||
Wilsonpage: I found the bug (1001510) that fixes the header alignments. What can we do to make sure this gets translated over to this bug?
Comment 31•10 years ago
|
||
amylee: A. If you want the hit-area to be reduced to *text only* it will likely have to be re-implemented. B. If you simply want the back arrow to turn blue when the header is pressed *without* changing the hit-area, it's super simple.
Flags: needinfo?(amlee)
Comment 32•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #31) > amylee: > > A. If you want the hit-area to be reduced to *text only* it will likely have > to be re-implemented. > B. If you simply want the back arrow to turn blue when the header is pressed > *without* changing the hit-area, it's super simple. The hit area will need to be changed based on the latest spec attached to this bug. So we'll have to re-implement it :-/
Flags: needinfo?(amlee)
Attachment #8417844 -
Flags: ui-review?
Attachment #8417844 -
Flags: ui-review+
Attachment #8417844 -
Flags: review?(wilsonpage)
Attachment #8417844 -
Flags: review-
Flags: needinfo?(ver4ffos)
Comment 33•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request Adding Tiff to review this
Attachment #8417844 -
Flags: ui-review?(tshakespeare)
Comment 34•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request The size of the tap area isn't to spec. It looks like the css:active state of the arrow isn't consistent, but I'm guessing that is the "regular" problem that we've been seeing in camera and not specific to this patch.
Attachment #8417844 -
Flags: ui-review?(tshakespeare) → ui-review-
Flags: needinfo?(tshakespeare)
Comment 35•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request The visuals look good. Thanks for fixing the alignment of the text :-) The press state isn't that responsive (i.e not turning blue all the time when you press it) but I think that is more a device issue than this patch. Thanks!
Attachment #8417844 -
Flags: ui-review?(amlee) → ui-review+
Updated•10 years ago
|
Attachment #8410778 -
Attachment is obsolete: true
Comment 36•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request Code looks good. This is up to amylee and tiff to make sure it meets their requirements, and also to make sure that it doesn't regress the head text alignment fix (ie. the Menu and Sub-Menu titles should have the same alignment so that they don't appear to jump when moving between menus).
Attachment #8417844 -
Flags: review?(wilsonpage) → review+
Comment 37•10 years ago
|
||
I think this might still be the case? (In reply to Tiffanie Shakespeare from comment #34) > Comment on attachment 8417844 [details] [review] > Pull request > > The size of the tap area isn't to spec. > > It looks like the css:active state of the arrow isn't consistent, but I'm > guessing that is the "regular" problem that we've been seeing in camera and > not specific to this patch.
Comment 39•10 years ago
|
||
I just installed the patch above to double check and the tap area isn't to spec (what Amy attached). If you can update this I will UI-reivew + the patch and hopefully it will land in 1.4. Thanks ver4ffos! (In reply to ver4ffos from comment #38) > Tiffanie, > > Is it going to land on v1.4 ?
Flags: needinfo?(tshakespeare)
Comment 40•10 years ago
|
||
Tiffanie, 1. According to spec given by Amy, the hit area is 290px wide and 45px in height. 2. 45px height can not cover the header so I made it to 90px. 3. Colored hit area shown in spec is about 150-160px in width. Tested on Device size: 360x640. Amy, please give your opinion.
Flags: needinfo?(tshakespeare)
Flags: needinfo?(amlee)
Comment 41•10 years ago
|
||
Comment 42•10 years ago
|
||
(In reply to ver4ffos from comment #40) > Tiffanie, > > 1. According to spec given by Amy, the hit area is 290px wide and 45px in > height. > 2. 45px height can not cover the header so I made it to 90px. > 3. Colored hit area shown in spec is about 150-160px in width. > > Tested on Device size: 360x640. > > Amy, please give your opinion. Hi, Thanks for fixing this. I'm good with the hit area size you provided. Thanks!
Flags: needinfo?(amlee)
Comment 43•10 years ago
|
||
You got it - sorry for that confusion! (In reply to ver4ffos from comment #40) > Tiffanie, > > 1. According to spec given by Amy, the hit area is 290px wide and 45px in > height. > 2. 45px height can not cover the header so I made it to 90px. > 3. Colored hit area shown in spec is about 150-160px in width. > > Tested on Device size: 360x640. > > Amy, please give your opinion.
Flags: needinfo?(tshakespeare)
Comment 44•10 years ago
|
||
what is next step? review- still there. Anything from my side please let me know.
Flags: needinfo?(tshakespeare)
Comment 45•10 years ago
|
||
Perhaps the branch wasn't updated? I just put the patch on my phone again and am still finding the tap area too large; it's still the whole area. It should be as you surmised in comment 43 - 90px x 150px. Once that's updated where good! Thanks for your patience! (In reply to ver4ffos from comment #44) > what is next step? > review- still there. > Anything from my side please let me know.
Flags: needinfo?(tshakespeare)
Attachment #8417844 -
Flags: ui-review?(tshakespeare)
Attachment #8417844 -
Flags: ui-review-
Attachment #8417844 -
Flags: review?(wilsonpage)
Attachment #8417844 -
Flags: review+
Updated•10 years ago
|
Attachment #8417844 -
Flags: review?(wilsonpage) → review+
Comment 46•10 years ago
|
||
Comment on attachment 8417844 [details] [review] Pull request Thanks so much!
Attachment #8417844 -
Flags: ui-review?(tshakespeare) → ui-review+
Comment 48•10 years ago
|
||
You need to squash commits before we can land this. Hema is this worth uplifting to v1.4? Seems too late and too minor a change IMO.
Flags: needinfo?(wilsonpage) → needinfo?(hkoka)
Comment 49•10 years ago
|
||
Cosmetic based on the original description, get this into master for 2.0. Only critical bugs are getting into 1.4 at this point. Thanks Hema
Flags: needinfo?(hkoka)
Target Milestone: --- → 2.0 S3 (6june)
Comment 52•10 years ago
|
||
ver4ffos: The patch is unable to be merged. You need to rebase against 'master'.
Flags: needinfo?(wilsonpage)
Comment 54•10 years ago
|
||
Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/4d3d20bee8d776278a48f6fd01c928e57e96b626
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Flags: needinfo?(wilsonpage)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•