Closed Bug 1077653 Opened 10 years ago Closed 10 years ago

Incoming call button icon spacing

Categories

(Hello (Loop) :: Client, defect, P2)

x86
All
defect
Points:
1

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx35+

People

(Reporter: shell, Assigned: tomasz)

References

Details

(Whiteboard: [UX])

Attachments

(2 files, 3 obsolete files)

feedback from ux review, #13 in the graphic https://bug1065441.bugzilla.mozilla.org/attachment.cgi?id=8497452
Flags: qe-verify-
Flags: firefox-backlog+
Summary: Incoming call button icon spacing → [UX] Incoming call button icon spacing
Whiteboard: [ux]
backlog: --- → Fx35+
Flags: qe-verify- → qe-verify?
Summary: [UX] Incoming call button icon spacing → Incoming call button icon spacing
Whiteboard: [ux]
Whiteboard: [UX]
Priority: -- → P2
I'll work on it. Btw, I'm pretty sure it shouldn't have [UX].
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
Points: --- → 1
Iteration: --- → 36.2
Attached patch incoming-call-spacing.patch (obsolete) — Splinter Review
So I added a slight start margin.

By the way we have the rtl broken in some places (like here: http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/browser/components/loop/standalone/content/css/webapp.css#l163).
Attachment #8513135 - Flags: review?(MattN+bmo)
Attached image after.png (obsolete) —
I have a feeling that now the spacing is too big.
(In reply to Tomasz Kołodziejski [:tomasz] from comment #3)
> Created attachment 8513140 [details]
> after.png
> 
> I have a feeling that now the spacing is too big.

Tomasz, please can you request ui-review on these types of patches - generally attach a screenshot  & request review from :darrin. This is layout they've already reviewed, so we need to make sure the new layout matches what they want.
Attachment #8513135 - Flags: review?(MattN+bmo) → review?(dmose)
Attached image after.png
Updated spacing for UI review.
Attachment #8513140 - Attachment is obsolete: true
Attachment #8513648 - Flags: review?(dhenein)
Comment on attachment 8513648 [details]
after.png

Spacing looks great.
Attachment #8513648 - Flags: review?(dhenein) → review+
Attached patch incoming-call-spacing.patch (obsolete) — Splinter Review
So there's one weird thing about our buttons. They don't have any padding set on the left and right (I was asked by :darrin to use that padding and so this investigation). It's only some internal (strange) firefox -moz-focus-inner spacing (padding and border, see http://stackoverflow.com/questions/5517744/remove-extra-button-spacing-padding-in-firefox). This results in inconsistency between firefox and chrome (see ui showcase which shows is it with "fake locale" with very long names. Normally buttons have min-width and center-aligned contents so this issue is invisible). So in this patch I made just a guess on the margin and I'm not fixing this inconsistency.

It's updated patch that produces UI-reviewed attachment 8513648 [details].
Attachment #8513135 - Attachment is obsolete: true
Attachment #8513135 - Flags: review?(dmose)
Attachment #8513652 - Flags: review?(dmose)
Comment on attachment 8513652 [details] [diff] [review]
incoming-call-spacing.patch

Review of attachment 8513652 [details] [diff] [review]:
-----------------------------------------------------------------

r=dmose, with the suggested tweak (or some variation) for easier understanding by future maintainers.

As far the moz-specific button stuff goes, I ran across another case recently where I felt like we might want to add some button-specific stuff to reset.css.  I'm little leery of runnig afoul of nsINativeTheme stuff, though...

::: browser/components/loop/content/shared/css/conversation.css
@@ +84,1 @@
>  }

Nice use of calc!  I'd speculate this would be even slightly more clear to the reader if it doubled down on that:

/* always leave space for the icon: 0.8rem is what looks visually
   reasonable; .2rem needs to match the value used for
   -moz-margin-start  */
max-width: calc(100% - 0.8rem - 0.2rem);

assuming I've understood the motivations correctly.  If I haven't, feel free to adjust the comment as you see fit.
Attachment #8513652 - Flags: review?(dmose) → review+
Updated to use calc even more explicitly. I somehow thought that the syntax is much more primitive and allows just one operation. Now I know that's not the case (http://www.w3.org/TR/css3-values/#calc-syntax).
Attachment #8513652 - Attachment is obsolete: true
Attachment #8513824 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a2812f546f76
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hi Tomasz, can you mark this bug for QE verification.
Flags: needinfo?(tkolodziejski)
I'd say it's a tiny change so -.
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(tkolodziejski)
Comment on attachment 8513824 [details] [diff] [review]
incoming-call-spacing.patch

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8513824 - Flags: approval-mozilla-aurora?
Attachment #8513824 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: