1069028 - Implement Loop panel footer layout/styling with FxA pieces

Status: RESOLVED FIXED

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

Description

[Matthew N. [:MattN]]

(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.

Comment 1

[:Paolo Amadini]

from UX review bug: https://bug1065441.bugzilla.mozilla.org/attachment.cgi?id=8497452 #7 in the graphic

Comment 3

[Matthew N. [:MattN]]

File to edit is at: https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/content/js/panel.jsx
CSS files are around there too.
See https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/README.txt for how to compile the React files and run tests.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

Comment 5

[Tomasz Kołodziejski [:tomasz]]

Attachment: after.png

This is a screenshot after the patch.

Comment 6

[Tomasz Kołodziejski [:tomasz]]

Attachment: 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.

Comment 7

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

Updated patch so that the user name is bold. Thanks :darrin!

Comment 8

[Tomasz Kołodziejski [:tomasz]]

Attachment: after.png

Updating screenshot.

Comment 9

[Matthew N. [:MattN]]

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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 11

[Tomasz Kołodziejski [:tomasz]]

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.

Comment 12

[Tomasz Kołodziejski [:tomasz]]

Attachment: Screen Shot 2014-10-20 at 11.14.08 AM.png

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.

Comment 13

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

I updated the patch as described in the comment 11. Please have a look.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8508097 [details] [diff] [review]
loop-panel-styling.patch

Shane, what do you think of the Social.jsm change?

Comment 15

[Shane Caraveo (: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.

Comment 16

[Tomasz Kołodziejski [:tomasz]]

Attachment: 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?

Comment 17

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 18

[Tomasz Kołodziejski [:tomasz]]

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?

Comment 19

[Matthew N. [:MattN]]

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?

Comment 20

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

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?

Comment 21

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 22

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

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).

Comment 23

[Shane Caraveo (:mixedpuppy)]

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.

Comment 24

[Tomasz Kołodziejski [:tomasz]]

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).

Comment 25

[Tomasz Kołodziejski [:tomasz]]

Probably not necessary but: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=49f7aaf2df03.

Comment 26

[Matthew N. [:MattN]]

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).

Comment 27

[Tomasz Kołodziejski [:tomasz]]

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.

Comment 28

[Matthew N. [:MattN]]

(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.

Comment 29

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-panel-styling.patch

Done.

Comment 32

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/52e8c44b21f0

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52e8c44b21f0

Comment 34

[Tomasz Kołodziejski [:tomasz]]

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.

Comment 35

[u279076]

(In reply to Carsten Book [:Tomcat] from comment #33)
> https://hg.mozilla.org/mozilla-central/rev/52e8c44b21f0

This is mozilla36, not 35.

Comment 36

[u279076]

I don't think this is high enough priority for QE verification. Any problems in the footer should be discoverable via routine smoketesting.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6414ad2ef81