1107048 - [FTE][RTL] Sim Manager SIM1 operator takes more vertical space than SIM2 operator

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Fernando Campo (:fcampo)]

Attachment: Sim Manager

I don't think it's specific for RTL but for any language that uses bigger descender heights on its font, but it's easier to see when changing to a RTL language

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Flex-box the SIM manager layout

Cant think of a good test for this. Fixes odd/variable heights in both LTR/RTL

Comment 2

[Fernando Campo (:fcampo)]

Comment on attachment 8535897 [details] [review]
Flex-box the SIM manager layout

Sadly, applying the changes result on the SIM images hidden (because of the flex property).
Removing it to show the images shows also that the height difference is still there (on Arabic language).

One possible solution would be to align the SIM details top with the image bottom (enough distance for big descenders on SIM-title), but that would need UX approval.

Comment 3

[Fernando Campo (:fcampo)]

Attachment: flex on off comparison

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8535897 [details] [review]
Flex-box the SIM manager layout

I've updated the PR with tweaks to the CSS. I actually wasn't able to repro your screenshot, but I did spot a couple flex-related mistakes. The updated patch seems to do reasonably well with both RTL/LTR and different line-heights and lengths. Due to the tall line-height forced by Arabic's diacritics, getting the image lined up with the first line of text is not trivial. I think this patch makes a reasonable compromise - I'll attach a screenshot.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Flex patch applied - en-us, arabic and accented english screenshots

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Flex patch applied - en-us, arabic and accented english screenshots

Comment 7

[Fernando Campo (:fcampo)]

Comment on attachment 8535897 [details] [review]
Flex-box the SIM manager layout

Yeah, this time working without problems. 
Not sure why I had the image hidden last time, but now I see it. 

Still discrepancies on the height for arabic, but I agree with you that to change that is not trivial, and we shouldn't be blocking on it. If UX is happy with the outcome, I'm happy too :)

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8536712 [details]
Flex patch applied - en-us, arabic and accented english screenshots

Does this look ok to you Eric? See comments, the main question is if the alignment of the icon to the first line is ok as-is.

Comment 9

[Eric Pang [:epang] UX]

Attachment: fte sim layout.png

Hey Sam,

I've taken a look at your screenshots and have a few notes to clean up the layout and alignment.

1. Vertically Center SIM card icons
2. SIM details should be left/right aligned with title
3. Spacing under SIM with Arabic character (4th screen) is inconsistent

More detail can be found in the attached image.  I've included what the screens will look like with the refinements at the bottom.  Let me know if you have any questions!

Sam, once these updates are made can you flag me for ui review again?
Thanks!

Comment 10

[Eric Pang [:epang] UX]

Comment on attachment 8536712 [details]
Flex patch applied - en-us, arabic and accented english screenshots

R- based on my previous comments, flag me again when ready, thanks Sam!

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: Flex patch applied - en-us, arabic and accented english screenshots

Updated to fix centered vertical alignment of icons and the alignment of SIM details. On the arabic screen, because of the mix of arabic and latin characters, the height of the line box is larger. If you draw a line from the bottom of the arabic letters you'll see the spacing is correct. I don't think we can fix this, its an artifact of the bidi/mixed-height content in that SIM label.

Comment 12

[Eric Pang [:epang] UX]

Comment on attachment 8537980 [details]
Flex patch applied - en-us, arabic and accented english screenshots

Thanks for updating the screens Sam!  Too bad we can't align to the sim text for Arabic text.  Regardless this looks much better.  R+

Comment 13

[Sam Foster [:sfoster] (he/him)]

Merged to master: https://github.com/mozilla-b2g/gaia/commit/4c230b117c55d987ae0ff71df9d4d8f080b93d3f

Comment 14

[Elie]

According comment 11 and 12
This issue verified successfully on Flame 2.2
Gaia-Rev        f5b3d1b6cfa3e702033f613915ae637cb735cbfb
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5d7497ce4cc7
Build-ID        20150120002507
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
Refer to picture

Comment 15

[Elie]

Attachment: 2015-01-20-18-22-35.png