1077470 - [Contacts] Add image from gallery to contacts needs a close (x) button instead of a back arrow

Status: RESOLVED FIXED

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

Description

[Amy Lee [:amylee] UX]

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

Comment 1

[Amy Lee [:amylee] UX]

Attachment: screenshot.png

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Moving to the gallery component

Comment 3

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Comment 4

[Amy Lee [:amylee] UX]

Comment on attachment 8501002 [details] [review]
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!

Comment 5

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Punam,
can you help me with "center the header text"? not sure why this doesn't work

Comment 6

[Punam Dahiya [:pdahiya]]

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!

Comment 7

[Wilson Page [:wilsonpage]]

(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;

Comment 8

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8501002 [details] [review]
patch for Gaia/master

Thanks Wilson :)

Hey Punam,
is it OK like this?

Comment 9

[Punam Dahiya [:pdahiya]]

(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

Comment 10

[Punam Dahiya [:pdahiya]]

Comment on attachment 8501002 [details] [review]
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!

Comment 11

[Wilson Page [:wilsonpage]]

(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.

Comment 12

[Punam Dahiya [:pdahiya]]

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!

Comment 13

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8501002 [details] [review]
patch for Gaia/master

Hey Punam,
thanks for the comments. I think now is ready for r?

Comment 14

[Punam Dahiya [:pdahiya]]

Comment on attachment 8501002 [details] [review]
patch for Gaia/master

Looks good. Setting ui-review flag for Amy. Thanks!

Comment 15

[Amy Lee [:amylee] UX]

Comment on attachment 8501002 [details] [review]
patch for Gaia/master

Looks good. Thanks!

Comment 16

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Thanks :)

Landed to master:
https://github.com/mozilla-b2g/gaia/commit/494cbb241e1500fa6aae9721e54ba4d0fbc66f11