865999 - After landing import from vCard Contact Settings screen is visually broken

Status: RESOLVED FIXED

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

Description

[Jose Manuel Cantera]

See the attached screen. The extra space between SDCard and the rest of options cause that FB Import / Update now appears scrolled and on the non-visible bottom of the screen which is totally non-sense. 

FB Import is an important functionality for the phone and now has been totally hijacked by more rarely used options, particularly import from vCard.

Comment 1

[Jose Manuel Cantera]

Attachment: Broken Contact Settings

Comment 2

[Jose Manuel Cantera]

My proposal to fix this is to rearrange the order of the different options and put those more rarely used at the bottom. Nonetheless, I'm worried about the scalability of the current Contact Settings approach while adding new import sources.

Comment 3

[Jose Manuel Cantera]

Attachment: Current implementation (v1-train)

Comment 4

[Jose Manuel Cantera]

Please check the new attachment that corresponds to v1-train version. It can be seen that the version in master is not only incorrect due to the extra space added by the SDcard option but also due to the extra space introduced on the left as it can be seen by comparing the two screen shots. 

This has to be fixed.

Comment 5

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: Github PR (Patch)

Comment 6

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 742376 [details] [diff] [review]
Github PR (Patch)

The actual pull: https://github.com/mozilla-b2g/gaia/pull/9431

Comment 7

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Can we get the r+ today? https://bugzilla.mozilla.org/show_bug.cgi?id=862742 is depending on this one. Thanks!

Comment 8

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 742376 [details] [diff] [review]
Github PR (Patch)

Looks great, good job!

Comment 9

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 742376 [details] [diff] [review]
Github PR (Patch)

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 #): 865999
User impact if declined: Misalignment of buttons in Contacts Settings screen and bad text formatting in case SD card not present.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): No risk.
String or UUID changes made by this patch: None.

Comment 10

[Jose Manuel Cantera]

I hate playing the role of devil's advocate by I still believe the patch is still incorrect, as the margin on the left hand side is considerably bigger than it was before these patches were applied.

Comment 11

[Sergi]

(In reply to Jose M. Cantera from comment #10)
> I hate playing the role of devil's advocate by I still believe the patch is
> still incorrect, as the margin on the left hand side is considerably bigger
> than it was before these patches were applied.

Agree wit you. We should review this.

Comment 12

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I see the space on the left now, but is this particular patch causing it for sure? If that is so I will check how can I fix it and resubmit it for review, approval, etc. again.

This patch was already approved and reviewed, and every time it takes a while to go through the whole process again. Please lets make sure that we catch all the visual glitches this time. Thanks!

Comment 13

[Jose Manuel Cantera]

(In reply to Sergi Mansilla from comment #12)
> I see the space on the left now, but is this particular patch causing it for
> sure? If that is so I will check how can I fix it and resubmit it for
> review, approval, etc. again.
> 
> This patch was already approved and reviewed, and every time it takes a
> while to go through the whole process again. Please lets make sure that we
> catch all the visual glitches this time. Thanks!

Approved and reviewed does not mean correctness. Compare with and without this patch and the other one from vCard import and you will feel the difference. The attached screens are very enlightening though.

Comment 14

[Maria Angeles Oteo (:oteo)]

If we can fix it before landing in mster,it's much better :) Otherwise we would need to create another follow-up bug.
Sergi, please, confirm that your patch is introdicing the space on the left and
- remove the gaia-approval-v1+
- create a new patch and after Jose Manuel revision
- request the gaia-approval-v1 again.
Thanks!

Comment 15

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Make sense if we leave this patch as is, and create a new bug for fixing the left margin? This is a minor css change and could be easy to make and land. Sorry for not noticing when I reviewed the patch.

Comment 16

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I agree with Arnau, since this patch has already landed in master.

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

It looks like this bug is not ready (or applicable) for uplifting but is flagged as needing uplift.  Please comment on this bug if something does need to be uplifted here.

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

The link to the pull request is actually the link to create a new pull request from comoyo's repository.  Please point me to the actual pull request or the commits from mozilla-b2g/gaia.git

Comment 19

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

Hi John,

perhaps the commit you are looking for is this one:

https://github.com/mozilla-b2g/gaia/commit/6f7897135f4cc70fb21eb3c737df4249a29c673b

Cheers,
F.

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 6f7897135f4cc70fb21eb3c737df4249a29c673b
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 21

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi John,

That's just a small merge conflict. I have it solved locally on the v1-train branch, should I push it to v1-train?

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Sergi Mansilla from comment #21)
> Hi John,
> 
> That's just a small merge conflict. I have it solved locally on the v1-train
> branch, should I push it to v1-train?

Yes, please.

Comment 23

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Merged - 8489f00f6e55c1acbf7f7d06ba0d6915096ec95a

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

$ git branch --contains 8489f00f6e55c1acbf7f7d06ba0d6915096ec95a
  v1-train