Closed
Bug 1077470
Opened 10 years ago
Closed 10 years ago
[Contacts] Add image from gallery to contacts needs a close (x) button instead of a back arrow
Categories
(Firefox OS Graveyard :: Gaia::Gallery, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: amylee, Assigned: pivanov)
References
Details
(Whiteboard: ux-tracking, visual design)
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-github-pull-request
|
pdahiya
:
review+
amylee
:
ui-review+
|
Details |
When selecting an image from gallery for a contacts entry, there should be an (x) icon to cancel and return to contacts instead of a back arrow. Back arrows are only used within one app to go back to the previous screen. The header text also needs to be centered. Steps to reproduce: 1. Open contacts app 2. Add new contact 3. Go to "Add Picture" and select gallery 4. Takes you to gallery to select an image. There should be an "x" instead of a back arrow and header should be centered. See attached screenshot. Please reference the "add contact" header in contacts app for correct "x" icon placement. Thanks
Reporter | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8501002 -
Flags: ui-review?(amlee)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Hi Pavel,
The "x" looks good. Can you please center the header text? I had mentioned it in the original bug but it might have been missed. Thanks!
Attachment #8501002 -
Flags: ui-review?(amlee) → ui-review-
Assignee | ||
Comment 5•10 years ago
|
||
Hey Punam, can you help me with "center the header text"? not sure why this doesn't work
Flags: needinfo?(pdahiya)
Comment 6•10 years ago
|
||
Hi Pavel One possible fix is to force header text by setting required margin #pick-header h1 { margin-right: 5rem !important; } However ideal fix should be to center-align h1 text in gaia-header web component itself. Setting NI flag for Wilson to provide his inputs on how it can be fixed in gaia-header. Also, I noticed we are repeating pick-header as id for h1 which is redundant and should be removed, https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/index.html#L135 . Thanks!
Flags: needinfo?(pdahiya) → needinfo?(wilsonpage)
Comment 7•10 years ago
|
||
(In reply to Punam Dahiya from comment #6) > Hi Pavel > One possible fix is to force header text by setting required margin This is not an option, as the text will be of different lengths depending on the localized string, also different margins would be needed fro different screen widths. Gaia-header should take care of the centering for you. It is not centering correctly it will be either: A. The header is `display: none` when it is 'created' meaning we can't do the measurements required. B. When the header was centered there were buttons (or other content) in the header that have since been hidden, making alignment appear incorrect. I believe this case looks like the latter. You can re-run the font-fit logic, once you know the header is visible by running the following on the header element: h1.textContent = h1.textContent;
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Thanks Wilson :)
Hey Punam,
is it OK like this?
Attachment #8501002 -
Flags: review?(pdahiya)
Comment 9•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #7) > (In reply to Punam Dahiya from comment #6) > > Hi Pavel > > One possible fix is to force header text by setting required margin > > This is not an option, as the text will be of different lengths depending on > the localized string, also different margins would be needed fro different > screen widths. > Agreed, not the right direction for this fix. > Gaia-header should take care of the centering for you. It is not centering > correctly it will be either: > > A. The header is `display: none` when it is 'created' meaning we can't do > the measurements required. > B. When the header was centered there were buttons (or other content) in the > header that have since been hidden, making alignment appear incorrect. https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/index.html#L133 , it's just header title and back button in this case. > I believe this case looks like the latter. You can re-run the font-fit > logic, once you know the header is visible by running the following on the > header element: > > h1.textContent = h1.textContent; While trying to see how it works in other apps, I noticed we trigger title to re-run font-fit logic for gaia-header whenever there is show/hide of buttons. https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/settings_utils.js#L83 In pick flow, it's not the gaia-header but containing pick-view that's set to display:none, could that be the reason? https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/index.html#L133
Flags: needinfo?(wilsonpage)
Comment 10•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Hi Pavel
I tested and patch does fix the issue, I am giving r- to move the logic inside startPick where it will be called only for pick flow. Thanks!
Attachment #8501002 -
Flags: review?(pdahiya) → review-
Assignee | ||
Updated•10 years ago
|
Attachment #8501002 -
Flags: review- → review?(pdahiya)
Comment 11•10 years ago
|
||
(In reply to Punam Dahiya from comment #9) > In pick flow, it's not the gaia-header but containing pick-view that's set > to display:none, could that be the reason? > https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/index.html#L133 Correct. Any descendents of a `display: none` element will also be hidden, meaning they are removed from the render tree, meaning it's not possible to read dimensions.
Flags: needinfo?(wilsonpage)
Comment 12•10 years ago
|
||
Hi Pavel Can you pl. address below comments on github and set review flag again. 1) Adding comments to explain why assigning textContent again is required. 2. Avoid repeat usage of $('pick-header-heading') Thanks!
Updated•10 years ago
|
Attachment #8501002 -
Flags: review?(pdahiya)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Hey Punam,
thanks for the comments. I think now is ready for r?
Attachment #8501002 -
Flags: review?(pdahiya)
Comment 14•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Looks good. Setting ui-review flag for Amy. Thanks!
Attachment #8501002 -
Flags: ui-review?(amlee)
Attachment #8501002 -
Flags: ui-review-
Attachment #8501002 -
Flags: review?(pdahiya)
Attachment #8501002 -
Flags: review+
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8501002 [details]
patch for Gaia/master
Looks good. Thanks!
Attachment #8501002 -
Flags: ui-review?(amlee) → ui-review+
Assignee | ||
Comment 16•10 years ago
|
||
Thanks :) Landed to master: https://github.com/mozilla-b2g/gaia/commit/494cbb241e1500fa6aae9721e54ba4d0fbc66f11
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•