Summary

Redesign the user infobox.

Review Request #8074 — Created March 24, 2016 and submitted April 21, 2016, 11:01 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

ae01554...

Reviewers

Groups

reviewboard

People

Description

This change is the first of two major improvements to the user infobox. This
one makes the infobox itself much lighter weight (with a lighter border color
and much lighter drop-shadow), and tweaks the design a bit.

This involves several changes:

Instead of listing the username and real name on separate lines, we now
prefer the real name if it's available, and put the username in parens on
the same line.
The avatar is now even bigger.
The avatar is flush with the sides of the infobox.
The user's local time is shown (computed by their preferred timezone).

There's also some code cleanup in here. I've created a new view for infoboxes,
which simplifies the code in common.es6.js. I also discovered while doing this
that the way we were fetching the infobox contents wouldn't send the
If-None-Match header. I've fixed that up, and made the client-side caching
better so if we have data cached client side, we'll show that immediately and
then update it when we get data back from the server.

This also updates the version of moment to the latest stable.

Testing Done

Looked at a bunch of user infoboxes and saw everything looked nice.

Files

Issues

Description	From	Last Updated
Can we add more padding? Maybe 1em? This is looking very crowded.	chipx86	April 6, 2016, 2:03 p.m.
'ChangeDescription' imported but unused	reviewbot	March 24, 2016, 4:18 p.m.
This should probably be a constant as we're also using it above in .user_infobox.	brennie	March 30, 2016, 2:43 p.m.
Doc comments throughout.	brennie	March 30, 2016, 2:47 p.m.
/* */	brennie	March 30, 2016, 2:44 p.m.
Mention in description that we're updating momentjs?	brennie	March 30, 2016, 2:48 p.m.
Blank line between these.	chipx86	April 15, 2016, 4:27 p.m.
Should we do UTC or configured server time?	chipx86	March 30, 2016, 5:53 p.m.
Multi-line comments should use: /* * */	chipx86	March 30, 2016, 6:37 p.m.
We chain this elsewhere, like: view.render().$el.appendTo(document.body); Minor thing, but saves some chars.	chipx86	March 30, 2016, 6:37 p.m.
Can we add literal quotes for <a>, so generated docs don't have issues?	chipx86	March 30, 2016, 6:37 p.m.
Same here.	chipx86	March 30, 2016, 6:37 p.m.
Blank line between these.	chipx86	March 30, 2016, 6:38 p.m.
Are we no longer doing one const/var/etc. keyword per grouping? I feel like consistency is good here.	chipx86	March 30, 2016, 5:51 p.m.
Blank line between these.	chipx86	March 30, 2016, 6:38 p.m.
Same question as above.	chipx86	March 30, 2016, 5:51 p.m.
Missing localization.	chipx86	April 15, 2016, 4:29 p.m.
Space before />. (Technically required for non-XML.)	chipx86	April 15, 2016, 4:29 p.m.
Missing localization.	chipx86	April 15, 2016, 4:29 p.m.
Missing localization.	chipx86	April 15, 2016, 4:29 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js

reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'ChangeDescription' imported but unused
```

reviewboard/static/rb/css/common.less (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably be a constant as we're also using it above in .user_infobox.
```

reviewboard/static/rb/js/common.es6.js (Diff revision 1)

You're not using a fat-arrow here because jQuery binds this to be the element, correct?

david March 30, 2016, 2:49 p.m.
```
Correct.
```

reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comments throughout.
```
reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
/* */
```
reviewboard/staticbundles.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Mention in description that we're updating momentjs?
```

Description:

		This change is the first of two major improvements to the user infobox. This
		one makes the infobox itself much lighter weight (with a lighter border color
		and much lighter drop-shadow), and tweaks the design a bit.

		This involves several changes:

		Instead of listing the username and real name on separate lines, we now prefer the real name if it's available, and put the username in parens on the same line.
		The avatar is now even bigger.
		The avatar is flush with the sides of the infobox.
		The user's local time is shown (computed by their preferred timezone).

		There's also some code cleanup in here. I've created a new view for infoboxes,
		which simplifies the code in common.es6.js. I also discovered while doing this
		that the way we were fetching the infobox contents wouldn't send the
		If-None-Match header. I've fixed that up, and made the client-side caching
		better so if we have data cached client side, we'll show that immediately and
		then update it when we get data back from the server.
	+
	+	This also updates the version of moment to the latest stable.

Commit:

-	069341ddb253889dffbc768481691bbcc2ecb036
+	7a0b0da392bcc429be365495e8bad452aafd9ea3

Diff:

Revision 2 (+5087 -111)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/views.py
	reviewboard/static/lib/js/moment-2.0.0.min.js
	reviewboard/static/lib/js/moment-2.12.0.js
	reviewboard/static/lib/js/moment-timezone-0.5.2.js
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/js/common.es6.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less

Ship it!

```
Ship It!
```

infobox.png (Revision 1)

The issue has been resolved. Show all issues

Can we add more padding? Maybe 1em? This is looking very crowded.

david March 30, 2016, 5:52 p.m.

I can't really add more padding without it overflowing the height of the box.

chipx86 March 30, 2016, 5:52 p.m.

That's going to be a problem for extensions then, won't it?

david March 30, 2016, 5:53 p.m.

Incidentally, adding more padding is one reason why I was hoping to drop the joined date.

chipx86 March 30, 2016, 5:55 p.m.

I think if padding breaks the design, we need to work more on the design before this can go in. We can't say "extension authors can now add additional content to the info box, but it will break if you want more than a line or two of text."

chipx86 March 30, 2016, 6:02 p.m.

Is the infobox actually a fixed height, or will it grow as content is added? If so, and this is about having just too much default content for us to be able to add padding, then we should rework the content. Maybe shrink the current time indicator and e-mail address. Hard to really tell how it'll look, because of the Retina image on the screenshot.

david March 30, 2016, 6:04 p.m.

Content added by extensions goes in a new section under both the image and the user data. Like this:

reviewboard/reviews/views.py (Diff revision 2)

The issue has been dropped. Show all issues

Should we do UTC or configured server time?

david March 30, 2016, 5:52 p.m.

All our config defaults to UTC (as does the field in the profile). I think the best solution would be to add some location-based timezone setting for all users.

reviewboard/static/rb/css/common.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Multi-line comments should use:

/*
 *
 */
```

reviewboard/static/rb/js/common.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

We chain this elsewhere, like:

view.render().$el.appendTo(document.body);


Minor thing, but saves some chars.

reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we add literal quotes for <a>, so generated docs don't have issues?
```
reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)

The issue has been dropped. Show all issues

Are we no longer doing one const/var/etc. keyword per grouping? I feel like consistency is good here.

david March 30, 2016, 5:52 p.m.

For var I think it's fine to group them because there we want "one statement at the top of the function" (and linters prefer that). For let/const I'm doing one per definition because they can appear anywhere.

reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/ui/views/infoboxView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Same question as above.
```

Commit:

-	7a0b0da392bcc429be365495e8bad452aafd9ea3
+	eb9330a005402683c82c03862f5507511f28243e

Diff:

Revision 3 (+5090 -111)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/views.py
	reviewboard/static/lib/js/moment-2.0.0.min.js
	reviewboard/static/lib/js/moment-2.12.0.js
	reviewboard/static/lib/js/moment-timezone-0.5.2.js
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/js/common.es6.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less

Commit:

-	eb9330a005402683c82c03862f5507511f28243e
+	64a2672478abbde552cb4fb01b8a01120ba4fa66

Diff:

Revision 4 (+5158 -116)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/views.py
	reviewboard/static/lib/js/moment-2.0.0.min.js
	reviewboard/static/lib/js/moment-2.12.0.js
	reviewboard/static/lib/js/moment-timezone-0.5.2.js
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/js/common.es6.js
	2 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less

reviewboard/reviews/views.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/templates/accounts/user_infobox.html (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
Missing localization.
```

reviewboard/templates/accounts/user_infobox.html (Diff revisions 2 - 4)

The issue has been resolved. Show all issues

Space before />. (Technically required for non-XML.)

david April 15, 2016, 4:22 p.m.

According to whom? If I'm reading the HTML5 spec correctly, that's not true.
That said, HTML5 seems to encourage "void tags" (like br) to not have closing tags at all, so I'll change it to be <br>

chipx86 April 15, 2016, 4:23 p.m.

Used to be the case at least. Some browsers would ignore the tag.

reviewboard/templates/accounts/user_infobox.html (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
Missing localization.
```
reviewboard/templates/accounts/user_infobox.html (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
Missing localization.
```

Commit:

-	64a2672478abbde552cb4fb01b8a01120ba4fa66
+	ae01554bad3405bd63c43a2cf73f371b57964355

Diff:

Revision 5 (+5164 -117)

Show changes

	reviewboard/staticbundles.py
	reviewboard/reviews/views.py
	reviewboard/static/lib/js/moment-2.0.0.min.js
	reviewboard/static/lib/js/moment-2.12.0.js
	reviewboard/static/lib/js/moment-timezone-0.5.2.js
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/js/common.es6.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/lib/js/moment-2.0.0.min.js
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/infoboxView.es6.js
    reviewboard/static/lib/js/moment-timezone-0.5.2.js
    reviewboard/static/lib/js/moment-2.12.0.js
    reviewboard/static/rb/css/defs.less

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (1577842)