917704 - Use Full Line Highlights in Settings

Status: RESOLVED FIXED

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

Description

[Ismael Gonzalez [:basiclines]]

Use full width highlights (edge to edge) for active/pressed states

Comment 1

[Ismael Gonzalez [:basiclines]]

Attachment: PR

Comment 2

[Eric Pang [:epang] UX]

Hi Fabien, can you review the patch Ismeal attached? 
Also, Arthur can you give feedback?  Thanks!

If possible i would like to get this into 1.2 since the highlight looks broken when it's not full length.  Nominated for Koi.

Comment 3

[Ivan Tsay (:ITsay)]

Need to fix it

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 806510 [details]
PR

Looks good to me. Thanks!

Comment 5

[Ismael Gonzalez [:basiclines]]

Arnau you may take care on testing this patch again and just push the merge button?
We have now f+ from Archur!

Comment 6

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

Ismael, no problem, I'll take this.

Comment 7

[Eric Pang [:epang] UX]

(In reply to Arnau March from comment #6)
> Ismael, no problem, I'll take this.

Hi Fabien, can you help review the patch so we can get this into 1.2?  Or is there someone you can pass the review to?  Thanks!

Comment 8

[Fabien Cazenave [:kaze]]

Comment on attachment 806510 [details]
PR

Works fine, no FPS were harmed in the process.

Merged on master: https://github.com/mozilla-b2g/gaia/commit/a72478ce2ecc3f1fff6a1158bad9ecd0c10bd914

Comment 9

[Eric Pang [:epang] UX]

(In reply to Fabien Cazenave [:kaze] from comment #8)
> Comment on attachment 806510 [details]
> PR
> 
> Works fine, no FPS were harmed in the process.
> 
> Merged on master:
> https://github.com/mozilla-b2g/gaia/commit/
> a72478ce2ecc3f1fff6a1158bad9ecd0c10bd914

Thanks Fabien!

Comment 10

[Jason Smith [:jsmith]]

First - this isn't a blocker for release - it's polish cleanup we've shipped with in past releases. 2nd - this caused an ugly VD regression in bug 929565, so this needs to be backed out of 1.2.

John - Can you back this out of 1.2?

Comment 11

[Eric Pang [:epang] UX]

(In reply to Jason Smith [:jsmith] from comment #10)
> First - this isn't a blocker for release - it's polish cleanup we've shipped
> with in past releases. 2nd - this caused an ugly VD regression in bug
> 929565, so this needs to be backed out of 1.2.
> 
> John - Can you back this out of 1.2?

Arnau, can you look into the VD regression this caused?  This is more then VD Polish... Currently the highlight state causes the UI to look broken.  In my opinion this is a very important fix.

Comment 12

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

Uplifted a72478ce2ecc3f1fff6a1158bad9ecd0c10bd914 to:
v1.2: d699497bbc7885a1b7e15956af7f7357badc7392

Comment 13

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

[v1.2 2de4944] Revert "Merge pull request #12292 from basiclines/settings-highlights"

Comment 14

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

We have fixed the regression in bug 929565.

Comment 15

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

Sorry for my previous comment, I just got he wrong bug number.
We are still working on that: 931693

Comment 16

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

Oh what a day! Sorry for that: Comment #14 was right, regression fixed in bug 929565.
Please ignore comment #15

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 806510 [details]
PR

Please uplift this patch.

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 806510 [details]
PR

This isn't safe for uplift & we're past the point of approvals. See bug 929565, which is a regression only fixed on master.