Closed Bug 1069028 Opened 5 years ago Closed 5 years ago

Implement Loop panel footer layout/styling with FxA pieces

Categories

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

defect
Points:
3

Tracking

(firefox34 wontfix, firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: MattN, Assigned: tomasz)

References

()

Details

(Whiteboard: [loop-uplift])

Attachments

(4 files, 7 obsolete files)

(Quoting Jared Wein [:jaws] from bug 1047146 comment #37)
> I didn't go much deeper with the styling as I'd like the styling of the
> panel to be split out in to a separate bug.

--

The panel footer is quite large and the layout of the footer containing the username, availability selector, sign in/up link and gear menu don't really match the spec.
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P2
Blocks: 1050307
Duplicate of this bug: 1077650
backlog: --- → Fx35+
backlog: Fx35+ → Fx34+
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
Attached patch loop-panel-styling.patch (obsolete) — Splinter Review
Attachment #8506272 - Flags: review?(MattN+bmo)
Iteration: --- → 36.1
Attached image after.png (obsolete) —
This is a screenshot after the patch.
Attached image after-clicked.png
After decreasing the size of the box there were some issues with the availability dropdown. I used bottom:0 and left:0 to align it instead of top:-37px or something.
Attached patch loop-panel-styling.patch (obsolete) — Splinter Review
Updated patch so that the user name is bold. Thanks :darrin!
Attachment #8506272 - Attachment is obsolete: true
Attachment #8506272 - Flags: review?(MattN+bmo)
Attachment #8506371 - Flags: review?(MattN+bmo)
Attached image after.png
Updating screenshot.
Attachment #8506334 - Attachment is obsolete: true
Comment on attachment 8506371 [details] [diff] [review]
loop-panel-styling.patch

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

When I logout I see: http://grab.by/Bmbg
You forgot the line on the left of the gear when logged in: http://grab.by/Bmbs – You can do this in a follow-up if you prefer.

::: browser/components/loop/content/shared/css/panel.css
@@ +344,5 @@
>  
>  .dnd-status {
>    border: 1px solid transparent;
>    padding: 2px 4px;
> +  margin: 0 0 0 -5px;

I think we prefer to do margin-left: -5px; instead of the four-value syntax if you aren't actually overriding the other 3 sides with 0.

@@ +381,3 @@
>    border-right: 1px solid #aaa;
>    padding-right: 1em;
> +  margin: 0 1em 0 0;

ditto
Attachment #8506371 - Flags: review?(MattN+bmo) → feedback+
Comment on attachment 8506371 [details] [diff] [review]
loop-panel-styling.patch

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

::: browser/components/loop/content/shared/css/panel.css
@@ +344,5 @@
>  
>  .dnd-status {
>    border: 1px solid transparent;
>    padding: 2px 4px;
> +  margin: 0 0 0 -5px;

These need RTL variants too.
Comment on attachment 8506371 [details] [diff] [review]
loop-panel-styling.patch

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

I will update this bit to support rtl. However, we have more rtl problems. Especially dropdowns seems to be out of place for rtl languages.

I've also noticed another problem which is: with too long email the menu will go out of the panel and you cannot log out.

As for the logout whitespace. We are using panel defined here http://hg.mozilla.org/mozilla-central/file/f2d7d694aae5/browser/modules/Social.jsm#l414. It has the PANEL_MIN_HEIGHT which is set to 200. In our case the height is 190 and so 10px of whitespace remains. dmose via IRC said that he talked to mixedpuppy in the past and it's ok to decrease the min height cap.
However, it was recently increased here http://hg.mozilla.org/mozilla-central/diff/1b1884049a65/browser/modules/Social.jsm. I'm not sure why that was needed (see the comment mentions error page size). I checked it and the min-size set to 0 is not too bad with the error page see the screenshot in the next comment.

::: browser/components/loop/content/shared/css/panel.css
@@ +344,5 @@
>  
>  .dnd-status {
>    border: 1px solid transparent;
>    padding: 2px 4px;
> +  margin: 0 0 0 -5px;

We are resetting top and bottom values here (there's global loop margin for paragraphs for some reason). I'll update it a little bit to support rtl and so that the separator line does not disappear for logged in users.
Error with PANEL_MIN_HEIGHT set to 0. If we want this to look better (like have bigger height) then we should add some margin on the body. So basically the PANEL_MIN_HEIGHT should be kept small and the auto resize should be enough to make it looks good.
Attached patch loop-panel-styling.patch (obsolete) — Splinter Review
I updated the patch as described in the comment 11. Please have a look.
Attachment #8506371 - Attachment is obsolete: true
Attachment #8508097 - Flags: review?(MattN+bmo)
Comment on attachment 8508097 [details] [diff] [review]
loop-panel-styling.patch

Shane, what do you think of the Social.jsm change?
Attachment #8508097 - Flags: review?(mixedpuppy)
Comment on attachment 8508097 [details] [diff] [review]
loop-panel-styling.patch

That's going to make the share panel too small.  We probably should make some way to pass in a default size for the different panels, or maybe put the defaults as attributes on the panel itself and retrieve those, fallback to the hardcoded values if the attributes dont exist.
Attachment #8508097 - Flags: review?(mixedpuppy)
Simply setting body {margin: 200px 0;} in /browser/themes/shared/aboutSocialError.css makes the above attachment. So I think that we don't need to use minimal size because the page can maintain its desired size by itself (like in this example by setting margin or min-height or whatever).

Thoughts?
Flags: needinfo?(mixedpuppy)
backlog: Fx34+ → Fx35+
(In reply to Tomasz Kołodziejski [:tomasz] from comment #16)
> Created attachment 8508388 [details]
> Screen Shot 2014-10-20 at 6.09.20 PM.png
> 
> Simply setting body {margin: 200px 0;} in
> /browser/themes/shared/aboutSocialError.css makes the above attachment. So I
> think that we don't need to use minimal size because the page can maintain
> its desired size by itself (like in this example by setting margin or
> min-height or whatever).
> 
> Thoughts?

jetlagged right now, forgive me if I'm being dense.

The error panel supports several different cases, I don't think you can handle all those cases this way.  I'm not sure if min-height would work there, if it does, that might be better.
Flags: needinfo?(mixedpuppy)
I could do that but it will be difficult for me to test all those several different cases I don't even know about. One more case that I saw in that panel is aboutNetError.xhtml which is itself far too big to fit in that panel.

Can you please list the error cases that I should test?
Flags: needinfo?(mixedpuppy)
Comment on attachment 8508097 [details] [diff] [review]
loop-panel-styling.patch

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

Looks good! r=me for the loop changes.

::: browser/components/loop/content/shared/css/panel.css
@@ +345,5 @@
>  .dnd-status {
>    border: 1px solid transparent;
>    padding: 2px 4px;
> +  margin: 0;
> +  -moz-margin-start: -5px;

To make it more clear where the number came from, can you make this a calc expression: calc(-1px - 4px)? A comment also wouldn't hurt so the numbers stay in sync.

@@ +473,5 @@
> +
> +.footer .signin-details {
> +  align-items: center;
> +  display: flex;
> +  -moz-margin-start: 5px;

Can you add a comment here too about how you came up with 5px?
Attachment #8508097 - Flags: review?(MattN+bmo) → review+
Attached patch loop-panel-styling.patch (obsolete) — Splinter Review
I'm commenting only the -5px because the second one you mentioned is arbitrary 5px in between flex "columns".

It looks like for us it's enough to decrease the min height by 10px. I still don't know how pixel-perfect 200 was chosen for social panel but hopefully we're not breaking it. I played with it a little bit and it looks ok. Checkin-needed?
Attachment #8508097 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8509631 - Flags: review+
(In reply to Tomasz Kołodziejski [:tomasz] from comment #18)
> I could do that but it will be difficult for me to test all those several
> different cases I don't even know about. One more case that I saw in that
> panel is aboutNetError.xhtml which is itself far too big to fit in that
> panel.
> 
> Can you please list the error cases that I should test?

aboutNetError shouldn't be showing up there, do you have an STR on it, make a bug for it?

The error page also shows up in the sidebar, you could use http://socialapi-demo.paas.allizom.org/ to see a sidebar, and probably create some test case around that.  The repo is here: https://github.com/mixedpuppy/socialapi-demo/  Another place it shows up is the chat windows, which iiuc are used by loop as well.

Using "min-height: 200px" (assuming that works at all, I haven't tried it) should be valid for any location the panel appears in.
Flags: needinfo?(mixedpuppy)
Attached patch loop-panel-styling.patch (obsolete) — Splinter Review
So I set the min-height in aboutSocialError.css. Can you please have a look mixedpuppy whether this works ok for your social use-cases? We'd like to get this patch landed and it's the last blocking bit for it. If it's not ok please point me to the place where it actually breaks things (screenshot welcomed).
Attachment #8509631 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo) → needinfo?(mixedpuppy)
Attachment #8509784 - Flags: review+
The new patch is 190px whereas the old patch was 160px, why the difference?  I may not be able to test the patch in the next couple days, if min-height is working, then this should be fine.
Flags: needinfo?(mixedpuppy)
It was enough for our stuff to make it 190px. So I changed it just to 190px instead of 160px so the potential impact is lower. As discussed on IRC we'll merge this (and post a follow-up bug if I missed some other panel issues).
Keywords: checkin-needed
Comment on attachment 8509784 [details] [diff] [review]
loop-panel-styling.patch

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

::: browser/components/loop/content/shared/css/panel.css
@@ +347,5 @@
>    padding: 2px 4px;
> +  margin: 0;
> +  -moz-margin-start: -5px; /* Undo the start border + padding so that unhovered
> +                              dnd-status is aligned as if there was no
> +                              additional spacing. */

Why didn't you use calc() too? It makes it more obvious that I need to change this if I change the 1px or 4px in the same block.

Also, we normally put long comments on the lines above (it would probably fit on two lines then).
I just felt like using calc to add 1 + 4 is an unnecessary work and an overkill. If you insist I can do that.

The reason for a comment being this way is http://hg.mozilla.org/mozilla-central/annotate/305d24174ace/browser/components/loop/content/shared/css/panel.css#l306. I can also change it if you want. Just let me know.
(In reply to Tomasz Kołodziejski [:tomasz] from comment #27)
> I just felt like using calc to add 1 + 4 is an unnecessary work and an
> overkill. If you insist I can do that.

I don't know about you but I don't read all comments near lines I'm changing as my mind is more interested in the code. I think the calc() is more important than the comment as it's clear that I need to check if I need to change it when I make another change.

> The reason for a comment being this way is
> http://hg.mozilla.org/mozilla-central/annotate/305d24174ace/browser/
> components/loop/content/shared/css/panel.css#l306. I can also change it if
> you want. Just let me know.

You shouldn't change the existing one but I think it would be nicer if you change your addition.
Done.
Attachment #8509784 - Attachment is obsolete: true
Attachment #8509984 - Flags: review+
Duplicate of this bug: 1088277
Duplicate of this bug: 1088281
https://hg.mozilla.org/integration/fx-team/rev/52e8c44b21f0
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [loop-uplift] → [loop-uplift][fixed-in-fx-team]
Attachment #8508081 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/52e8c44b21f0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [loop-uplift][fixed-in-fx-team] → [loop-uplift]
Target Milestone: --- → mozilla35
Comment on attachment 8509984 [details] [diff] [review]
loop-panel-styling.patch

Approval Request Comment
[Feature/regressing bug #]: Improved styling of the footer of the loop panel (this = bug 1069028).
[User impact if declined]: Unpleasant styling of the main loop panel as in picture #8 in attachment 8497452 [details].
[Describe test coverage new/current, TBPL]: Manual testing due to the visual nature of the bug.
[Risks and why]: Low risk due to the visual nature of the bug.
[String/UUID change made/needed]: None.
Attachment #8509984 - Flags: approval-mozilla-aurora?
(In reply to Carsten Book [:Tomcat] from comment #33)
> https://hg.mozilla.org/mozilla-central/rev/52e8c44b21f0

This is mozilla36, not 35.
Target Milestone: mozilla35 → mozilla36
I don't think this is high enough priority for QE verification. Any problems in the footer should be discoverable via routine smoketesting.
Flags: qe-verify+ → qe-verify-
Depends on: 1089191
Attachment #8509984 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.