Closed Bug 943165 Opened 10 years ago Closed 9 years ago

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


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

Gonk (Firefox OS)
Not set


(b2g-v2.2 fixed)

2.1 S9 (21Nov)
Tracking Status
b2g-v2.2 --- fixed


(Reporter: julienw, Assigned: munto, Mentored)


(Whiteboard: [lang=css][good first bug])


(5 files, 1 obsolete file)

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.

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 :)
Flags: needinfo?(vittone)

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.
Flags: needinfo?(vittone)
Okay thanks. I'll see why we don't have the normal BB values here then (esp the font-size and line-height)!
Assignee: felash → nobody
Mentor: felash
Whiteboard: [lang=css][good first bug]
Attached file Github pull request
Assignee: nobody → 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! :)
Attachment #8515503 - Attachment description: Github pullrequest → Github pull request
Attachment #8515503 - Flags: review?(felash)
Attached image before - after (obsolete) —
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?
Attachment #8515987 - Flags: ui-review?(fshih)
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!
Attachment #8515503 - Flags: review?(felash)
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!
Attachment #8515987 - Flags: ui-review?(fshih) → ui-review-
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.
Flags: needinfo?(felash)
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? :)
Flags: needinfo?(felash) → needinfo?(fshih)
Attached image 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.
Flags: needinfo?(fshih)
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 :)
Attachment #8515503 - Flags: review?(felash)
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.
Attachment #8515987 - Attachment is obsolete: true
Attachment #8517447 - Flags: ui-review?(fshih)
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 !
Attachment #8515503 - Flags: review?(felash)
Comment on attachment 8517447 [details]
contact prompt - unknown contact prompt

Thanks for the suggestion. Looks great!!
Attachment #8517447 - Flags: ui-review?(fshih) → ui-review+
Attached file 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 :)
Attachment #8519765 - Flags: review?(azasypkin)
Comment on attachment 8519765 [details] [review]
rebased github PR

Looks good, thank you guys for handling this!
Attachment #8519765 - Flags: review?(azasypkin) → review+
rebased, squashed, let's land when the tree reopens!
Keywords: checkin-needed
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
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 :)
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 :)
You need to log in before you can comment on or make changes to this bug.