943165 - [Messages] Bug 934531 follow-up: make the visual right for numbers-only headers

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Attachment: SMS.MMS.Contact_with_photo_SPEC.png

See attachment for the current spec.

Bug 934531 will make it right for the contact prompt, but not for the number prompt.

This bug will likely need to change how the contact prompt is done too, but this should be painless.

Comment 1

[Julien Wajsberg [:julienw]]

José,

can you make the padding and line-height in the "numbers" case explicit?

What I think is that the header should be 5rem, which leaves 4.9rem for the content because of the 1px line.

That means the text would have both a font-size and line-height of 2.3rem, and the top and bottom paddings would be 1.3rem.

Please tell me if I'm correct here :)
Thanks!

Comment 2

[José Vittone]

Julien,

I didn't specify it because it is currently on Building Blocks, like any other header. 

They are aligned this way:
height: 5rem;
line-height: 4.9rem; // no paddings at all
font-size: 2.3rem; // our only difference is the font size and weight


Having said that, measurements you describe also seem to work as well.

Comment 3

[Julien Wajsberg [:julienw]]

Okay thanks. I'll see why we don't have the normal BB values here then (esp the font-size and line-height)!

Comment 4

[Dorian [:munto]]

Attachment: Github pull request

Comment 5

[Dorian [:munto]]

Comment on attachment 8515503 [details] [review]
Github pull request

Here is my first pull request after a weekend of bug squashing with Julien.
I really appreciated to have a private mentor.
Thank you to him! :)

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: before - after

Hey Fang,

Here you can see
* the implementation for a prompt for a known contact
* the old visual for an unknown contact
* the new visual for an unknown contact according the the slightly old spec

What do you think?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8515503 [details] [review]
Github pull request

I left some comments on github, where I ask you to change the class name, and to resolve the conflict.

Please ping me if you don't have time to do this and I can take it from here :) Otherwise, please flag me for review again once you're ready !

Thanks a lot!

Comment 8

[Fang Shih [:fang]]

Comment on attachment 8515987 [details]
before - after

Hey Julien,

The padding and the font size look good! The only thing I am concerned is the font weight. From the old spec is FiraSans Light. And from this patch, The font looks a bit lighter than that. Just want to make sure it is using the "Fira Sans Light" instead of UltraLight. Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

I used Firefox to do the screenshots, so the fonts could be wrong because of this. I'll double check on a device and post new screenshots.

Comment 10

[Julien Wajsberg [:julienw]]

Hey Fang,

I checked, and we use a "font-weight: 100" property for this header. Maybe we should use "200" to use the "Light" version. I don't really know how fonts are selected from the font-weight, but maybe you know, or you know who can know? :)

Comment 11

[Fang Shih [:fang]]

Attachment: Font Weight.png

Hi Julien,

I just checked, The "font-weight" for Fira Sans Light would be "300". 
Let me know if you need any info, Thanks : ) 

Attached is our typeface guideline for CSS.

Comment 12

[Dorian [:munto]]

Comment on attachment 8515503 [details] [review]
Github pull request

Hi Julien.
Thank you for taking the time to explain all these things in details.

I did what you wrote on github and it seems to work.
I had a little issue though. When I resolved the conflict I left the lines with diples (<<<) because I thought git would remove them automatically during rebase --continue

I tried to redo a rebase to remove those lines from the commit but they were still there. I don't know how to do it without losing my modifications.
So if you can manage it for me, it would be a relief :)

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: contact prompt - unknown contact prompt

Hey Fang,

does that look better? I took it from a Flame this time.

This is the result of the patch with my suggested change, so if you flash the current patch it won't have the exact same result.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8515503 [details] [review]
Github pull request

Hey Dorian,

I suggested a different fix for the font-weight, and there are some linter issues in the test file.

You can see the linter issues when you follow the link that is automatically added when you push ("Munto Munto (Munto) started tests. Results"). You can also run them locally: "APP=sms make hint". They're supposed to be run automatically at commit time but I just saw we have an issue in our commit hook (bug 1094190) :)

You can add a separate commit with your new changes and then push to your PR :) When you're ready please ask review again !

Thanks !

Comment 15

[Fang Shih [:fang]]

Comment on attachment 8517447 [details]
contact prompt - unknown contact prompt

Thanks for the suggestion. Looks great!!

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: rebased github PR

Hey Oleg,

since this changes some of your changes for BiDi functionality, can you have a last look on this?

Dorian, I took the liberty to rebase your patch on latest master :) The polishing is quite both boring and uneasy (when you don't know the other patches that landed) so I'll do it and land your patch for you, if you don't mind. You'll still be listed as the author of the patch obviously :)

Comment 17

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8519765 [details] [review]
rebased github PR

Looks good, thank you guys for handling this!

Comment 18

[Julien Wajsberg [:julienw]]

rebased, squashed, let's land when the tree reopens!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/ae2e8e79abf972ce3fa647880e6edc297bfd8236

Comment 20

[Dorian [:munto]]

Hello Julien,

Thank you for taking care of applying this patch.
In my last commit, I saw so many unwanted changes. I didn't know how to remove them.
I admit that I didn't really master what I was doing with all these rebases.

Bug fixed ! It's cool :)

Comment 21

[Julien Wajsberg [:julienw]]

Yep, when rebasing, you get all the new changes, and all your own changes, and at each conflict location, you need to manually decide which changes to keep.

Most of the time you need to take the new changes, and change them again according to your own changes. But this requires to understand what the new changes do (usually, you'll need to use "git blame" and go to the bug that these changes belong to). So that's not really easy for a newcomer :)