Closed
Bug 1077653
Opened 10 years ago
Closed 10 years ago
Incoming call button icon spacing
Categories
(Hello (Loop) :: Client, defect, P2)
Tracking
(firefox35 fixed, firefox36 fixed)
backlog | Fx35+ |
People
(Reporter: shell, Assigned: tomasz)
References
Details
(Whiteboard: [UX])
Attachments
(2 files, 3 obsolete files)
15.26 KB,
image/png
|
dhenein
:
review+
|
Details |
1.31 KB,
patch
|
tomasz
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
feedback from ux review, #13 in the graphic https://bug1065441.bugzilla.mozilla.org/attachment.cgi?id=8497452
Updated•10 years ago
|
Flags: qe-verify-
Flags: firefox-backlog+
Summary: Incoming call button icon spacing → [UX] Incoming call button icon spacing
Whiteboard: [ux]
Reporter | ||
Updated•10 years ago
|
backlog: --- → Fx35+
Updated•10 years ago
|
Flags: qe-verify- → qe-verify?
Summary: [UX] Incoming call button icon spacing → Incoming call button icon spacing
Whiteboard: [ux]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [UX]
Reporter | ||
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•10 years ago
|
||
I'll work on it. Btw, I'm pretty sure it shouldn't have [UX].
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
Points: --- → 1
Updated•10 years ago
|
Iteration: --- → 36.2
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
I have a feeling that now the spacing is too big.
Comment 4•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8513135 -
Flags: review?(MattN+bmo) → review?(dmose)
Assignee | ||
Comment 5•10 years ago
|
||
Updated spacing for UI review.
Attachment #8513140 -
Attachment is obsolete: true
Attachment #8513648 -
Flags: review?(dhenein)
Comment 6•10 years ago
|
||
Comment on attachment 8513648 [details]
after.png
Spacing looks great.
Attachment #8513648 -
Flags: review?(dhenein) → review+
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8513652 -
Flags: review?(dmose) → review+
Assignee | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a2812f546f76
Keywords: checkin-needed
Target Milestone: --- → mozilla36
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a2812f546f76
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 12•10 years ago
|
||
Hi Tomasz, can you mark this bug for QE verification.
Flags: needinfo?(tkolodziejski)
Assignee | ||
Comment 13•10 years ago
|
||
I'd say it's a tiny change so -.
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(tkolodziejski)
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
Comment 15•10 years ago
|
||
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?
Updated•10 years ago
|
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.
Description
•