Implement Loop panel footer layout/styling with FxA pieces

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: MattN, Assigned: tomasz)

Tracking

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

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [loop-uplift], URL)

Attachments

(4 attachments, 7 obsolete attachments)

(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

Comment 1

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

Updated

3 years ago
Blocks: 1050307

Updated

3 years ago
Duplicate of this bug: 1077650

Updated

3 years ago
backlog: --- → Fx35+

Updated

3 years ago
backlog: Fx35+ → Fx34+
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Created attachment 8506272 [details] [diff] [review]
loop-panel-styling.patch
Attachment #8506272 - Flags: review?(MattN+bmo)

Updated

3 years ago
Iteration: --- → 36.1
(Assignee)

Comment 5

3 years ago
Created attachment 8506334 [details]
after.png

This is a screenshot after the patch.
(Assignee)

Comment 6

3 years ago
Created attachment 8506335 [details]
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8506371 [details] [diff] [review]
loop-panel-styling.patch

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)
(Assignee)

Comment 8

3 years ago
Created attachment 8506374 [details]
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.
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Comment 12

3 years ago
Created attachment 8508081 [details]
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.
(Assignee)

Comment 13

3 years ago
Created attachment 8508097 [details] [diff] [review]
loop-panel-styling.patch

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)
(Assignee)

Comment 16

3 years ago
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?
Flags: needinfo?(mixedpuppy)

Updated

3 years ago
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)
(Assignee)

Comment 18

3 years ago
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+
(Assignee)

Comment 20

3 years ago
Created attachment 8509631 [details] [diff] [review]
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?
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)
(Assignee)

Comment 22

3 years ago
Created attachment 8509784 [details] [diff] [review]
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).
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)
(Assignee)

Comment 24

3 years ago
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
(Assignee)

Comment 25

3 years ago
Probably not necessary but: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=49f7aaf2df03.
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).
(Assignee)

Comment 27

3 years ago
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.
(Assignee)

Comment 29

3 years ago
Created attachment 8509984 [details] [diff] [review]
loop-panel-styling.patch

Done.
Attachment #8509784 - Attachment is obsolete: true
Attachment #8509984 - Flags: review+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1088277
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [loop-uplift][fixed-in-fx-team] → [loop-uplift]
Target Milestone: --- → mozilla35
(Assignee)

Comment 34

3 years ago
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.
status-firefox34: --- → wontfix
status-firefox35: --- → affected
status-firefox36: --- → fixed
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-

Updated

3 years ago
Depends on: 1089191
Attachment #8509984 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6414ad2ef81
status-firefox35: affected → fixed
Depends on: 1098517
You need to log in before you can comment on or make changes to this bug.