Margins and paddings are inconsistent/broken since bug 1074672

RESOLVED FIXED in Firefox 35

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

({regression})

unspecified
mozilla36
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [UX bug])

Attachments

(4 attachments, 6 obsolete attachments)

10.05 KB, image/png
Details
16.90 KB, image/png
Details
2.92 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
6.50 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Posted image Screenshot of bug
See attached screenshot.

The "No current conversations" overlaps the "Beta" tag, and the terms of service are touching the borders of the panel.

Further, the "No current conversations" is indented .5rem, but the "Guest" string is indented 14px. Same for "Available" and the available-dropdown.
Posted image Another screenshot
The rooms list is also inconsistent.
jared going to look at this one - pretty ugly.
backlog: --- → Fx35+
Priority: -- → P1
Whiteboard: [UX bug]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 8
Flags: qe-verify-
Flags: firefox-backlog+
Posted patch Patch (obsolete) — Splinter Review
Attachment #8526229 - Flags: review?(mdeboer)
Posted patch Patch v1.1 (obsolete) — Splinter Review
Fixed a margin under the Start a Conversation button.
Attachment #8526229 - Attachment is obsolete: true
Attachment #8526229 - Flags: review?(mdeboer)
Attachment #8526256 - Flags: review?(mdeboer)
Posted patch Patch v1.1.1 (obsolete) — Splinter Review
Sorry, a line crept in from another patch.
Attachment #8526256 - Attachment is obsolete: true
Attachment #8526256 - Flags: review?(mdeboer)
Attachment #8526260 - Flags: review?(mdeboer)
Comment on attachment 8526260 [details] [diff] [review]
Patch v1.1.1

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

Pretty big, risky patch, but overall this looks like the right direction.

You have to iterate it once more, though, to make sure that the footer stays where it should be in the rooms view (see attached screenshot)
Attachment #8526260 - Flags: review?(mdeboer) → review-
Posted patch Patch v1.2 (obsolete) — Splinter Review
I added back the min-height.
Attachment #8526260 - Attachment is obsolete: true
Attachment #8526789 - Attachment is obsolete: true
Attachment #8526930 - Flags: review?(mdeboer)
Posted patch Patch v1.2 (rebased) (obsolete) — Splinter Review
Attachment #8526930 - Attachment is obsolete: true
Attachment #8526930 - Flags: review?(mdeboer)
Attachment #8526983 - Flags: review?(mdeboer)
From your request in bug 1101754, what do you think about this patch? Here is a screenshot (partner logo covered), http://screencast.com/t/CeJ1jXK4v

This doesn't vertically center the contents, but moves the ToS section to be aligned to the bottom of the panel. I couldn't find a way to get these contents vertically centered, but this looks okay to me.
Attachment #8527009 - Flags: review?(mdeboer)
Comment on attachment 8526983 [details] [diff] [review]
Patch v1.2 (rebased)

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

One issue left: when you hover a contact, the call button does not cover the 'Google' icon, which is visible for imported contacts.

::: browser/components/loop/content/shared/css/panel.css
@@ +233,5 @@
>  }
>  
>  .room-list > .room-entry > p {
>    margin: 0;
> +  padding: .2em 0;

Shouldn't this be `rem` too?
Attachment #8526983 - Flags: review?(mdeboer) → review-
Comment on attachment 8527009 [details] [diff] [review]
Patch part 2 (fix vertical alignment for Getting Started + ToS view)

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

LGTM :)
Attachment #8527009 - Flags: review?(mdeboer) → review+
Posted patch Patch v1.2.1Splinter Review
With the Google icon fixed so it hides behind the action button when the contact is hovered, and the margins switched from em to rem on the .room-list > .room-entry > p.
Attachment #8526983 - Attachment is obsolete: true
Attachment #8527951 - Flags: review?(mdeboer)
Iteration: 36.3 → 37.1
Comment on attachment 8527951 [details] [diff] [review]
Patch v1.2.1

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

Ship it!
Attachment #8527951 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/2e46648a150c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8527951 [details] [diff] [review]
Patch v1.2.1

Approval Request Comment
[Feature/regressing bug #]: bug 1074672

[User impact if declined]: Visual bugs (overlaps, etc) - see images

[Describe test coverage new/current, TBPL]: manual testing; easily visible

[Risks and why]: Low risk - purely visual CSS changes and adding an id to a div

[String/UUID change made/needed]: none
Attachment #8527951 - Flags: approval-mozilla-aurora?
Attachment #8527009 - Flags: approval-mozilla-aurora?
Is it possible to have automated tests (maybe a reftest) to cover this, since it is a regression?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #18)
> Is it possible to have automated tests (maybe a reftest) to cover this,
> since it is a regression?

Not really. We have dealt with similar issues in other places of the Firefox UI, and due to platform and environment differences we're unable to do things like reftests for chrome UI reliably.
Flags: in-testsuite-
Flags: in-moztrap-
Comment on attachment 8527009 [details] [diff] [review]
Patch part 2 (fix vertical alignment for Getting Started + ToS view)

Approval Request Comment
Move request to beta
Attachment #8527009 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8527951 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8527009 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8527951 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Iteration: 37.1 → 36.3
Iteration: 36.3 → 37.1
You need to log in before you can comment on or make changes to this bug.