1056377 - [rbmozui] Mozilla-ify the UI

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

We want our instance of Review Board to feel "like home" to people working on Mozilla stuff. We're going to take advantage of the extensibility / customizability of Review Board to meet that goal.

This bug will track that work.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Review for review ID: bz://1056377/mconley

/r/126 - First go at re-skinning the RB menus.
/r/127 - Switch to FontAwesome.

Pull down these commits:

hg pull review -r fd4668ecfac74e735c23ccab0b359d5d7587018d

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Note that the patches I just posted are very much a WIP.

Comment 3

[Steven MacLeod [:smacleod]]

I'd love to see some screenshots along with the patches as we go :)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Sure - I'll toss some up soon!

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8476319 [details]
Review for review ID: bz://1056377/mconley

/r/126 - First go at re-skinning the RB menus.
/r/127 - Switch to FontAwesome.

Pull down these commits:

hg pull review -r fd4668ecfac74e735c23ccab0b359d5d7587018d

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8476319 [details]
Review for review ID: bz://1056377/mconley

/r/126 - First go at re-skinning the RB menus.
/r/127 - Switch to FontAwesome.

Pull down these commits:

hg pull review -r fd4668ecfac74e735c23ccab0b359d5d7587018d

Comment 7

[Steven MacLeod [:smacleod]]

(Since screenshot comments are broken right now, I'll just explain). It looks like the description box is using the stock Review Board colour? It seems a little out of place to me, we should modify it.

Comment 8

[Steven MacLeod [:smacleod]]

the "description box" _border_ is what I mean to say ^

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Good eye!

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8476319 [details]
Review for review ID: bz://1056377/mconley

/r/127 - First go at re-skinning Review Board for Mozilla.

Pull down this commit:

hg pull review -r a6c77836943e1729271f5765efbd1731bd6b1e2f

Comment 11

[Gregory Szorc [:gps]]

This seems to be almost ready to land. Higher-than normal priority so we don't lose track of it.

Comment 12

[Mark Côté [:mcote]]

Transferring to mdoglio.  If mconley wants/is able to help out, that's cool, but we'll get mdoglio to drive this to completion.

Comment 13

[Mark Côté [:mcote]]

We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.

Comment 14

[Mauro Doglio [:mdoglio]]

We decided that we will hold on this because the next ReviewBoard release may ship with support for ui themes.

Comment 15

[Mauro Doglio [:mdoglio]]

I think glob is going to work on it this quarter

Comment 16

[David Walsh :davidwalsh]

Attachment: MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

Review commit: https://reviewboard.mozilla.org/r/51419/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51419/

Comment 17

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/51419/#review48183

This was a quick experiment with how we could improve MozReview's design.  I used Sean Martell's color palette (https://www.mozilla.org/en-US/styleguide/identity/firefox/color/) and attempted a very "toned down" look so it's easy on the eyes.  I did *not* address font-sizing or line-height; if we think this is the right direction, I can adjust those down the road.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Thanks for working on this, David!

Any chance you could post a quick screenshot? (I'm currently not in the position to apply and test your patch myself.)

Comment 19

[David Walsh :davidwalsh]

Attachment: all-requests.png

Comment 20

[David Walsh :davidwalsh]

Attachment: review.png

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8750448 [details]
MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

https://reviewboard.mozilla.org/r/51419/#review48235

Gave it a brief glance over and it looks so much better. +100 for less yellow.

I'm no CSS expert, so I can't comment much on the technical aspects or on the implications of overriding CSS in Review Board.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

I agree, this looks really really nice.

One thing that sticks out is the Firefox logo - that's the old logo, and I can say with 100% certainty that mart3ll would die inside if we used that instead of the most up-to-date one from [1].

Alternatively, since there are non-Firefox projects on MozReview, it might be worth-while trying out the Mozilla wordmark from [2] as well.

Might want to confirm with mcote or somebody that "Mozilla Review Board" is how we want to title this thing. Maybe "MozReview" or "Mozilla's Review Board" or maybe just "Mozilla Code Review". Dunno. Start your bikeshed engines.

Regarding overriding the CSS in Review Board, it should be able to handle whatever you throw at it, for the most part.

[1]: https://www.mozilla.org/en-US/styleguide/identity/firefox/branding/
[2]: https://www.mozilla.org/en-US/styleguide/identity/mozilla/branding/

Comment 23

[Mark Côté [:mcote]]

Yeah I was vacillating on the title.  "MozReview" is the name of the entire system, which encompasses not just the Review Board instance but also the hg repo and, arguably, the Autoland service.  Since we haven't significantly diverged from Review Board, it's probably not a bad idea to keep it in the title.  So, yeah, I guess to me, at the moment, it's between "Mozilla Review Board" and "Mozilla's Review Board".  But I'm open to bike shedding.

Comment 24

[:glob ✱]

we could follow bmo.. "Review Board @ Mozilla"

Comment 25

[:glob ✱]

Comment on attachment 8750448 [details]
MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

https://reviewboard.mozilla.org/r/51419/#review48435

i love the overall look; nice work!

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:19
(Diff revision 1)
> +  padding-top: 8px;
> +}
> +
> +
> +/* TODO:  Pull this from our own resources/CDN */
> +@import url(https://fonts.googleapis.com/css?family=Open+Sans:400,700);

opening issue for this TODO

i've asked around to see if there's a copy of this on our cdn.  if there isn't, we should host a copy on our review board servers.

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:103
(Diff revision 1)
> +  -webkit-border-radius: 0;
> +  -moz-border-radius: 0;

nit: we probably don't need to worry about supporting ancient versions of webkit and gecko, and only need border-radius (drop -webkit and -moz prefixed definitions).  eg. -moz-border-radius was removed in firefox 13

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:217
(Diff revision 1)
> +.actions.page-tabs li {
> +  border-bottom: 0;
> +}
> +
> +.actions.page-tabs li.active {
> +  border: 0;

you need a background-color: #fff here; currently the active tab has a faint yellow background

::: pylib/mozreview/mozreview/static/mozreview/js/default.js:9
(Diff revision 1)
>  
>    // Fix logout url
>    $("a[href='/account/logout/']").attr("href", "/mozreview/logout/");
>    $("#accountnav li").css("visibility", "visible");
> +
> +  /* DW attempt at a redesign */

this comment should say something along the lines of "use the mozilla/firefox logo"

::: pylib/mozreview/mozreview/static/mozreview/js/default.js:10
(Diff revision 1)
>    // Fix logout url
>    $("a[href='/account/logout/']").attr("href", "/mozreview/logout/");
>    $("#accountnav li").css("visibility", "visible");
> +
> +  /* DW attempt at a redesign */
> +  $('#logo').attr('src', 'http://people.mozilla.org/~faaborg/files/shiretoko/firefoxIcon/firefox-512-noshadow.png');

as per comment 22, this is the old firefox logo.

the logo should be served from the review board webheads, not from another host.

Comment 26

[David Walsh :davidwalsh]

Attachment: MozReview Request: Update diaog design

Review commit: https://reviewboard.mozilla.org/r/51677/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51677/

Comment 27

[David Walsh :davidwalsh]

Attachment: MozReview Request: Update comment dialog box

Review commit: https://reviewboard.mozilla.org/r/51679/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51679/

Comment 28

[Mark Côté [:mcote]]

Comment on attachment 8750448 [details]
MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

https://reviewboard.mozilla.org/r/51419/#review48989

I think this looks pretty grand, but I will leave specific comments to glob and others.

Comment 29

[David Walsh :davidwalsh]

Attachment: MozReview Request: Add new logo and styles for logo display

Review commit: https://reviewboard.mozilla.org/r/53682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53682/

Comment 30

[David Walsh :davidwalsh]

Attachment: MozReview Request: Re-skin MozReview (Bug 1056377). r?smacleod

Review commit: https://reviewboard.mozilla.org/r/53686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53686/

Comment 31

[David Walsh :davidwalsh]

Attachment: MozReview Request: Update page heading content

Review commit: https://reviewboard.mozilla.org/r/53688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53688/

Comment 32

[David Walsh :davidwalsh]

Attachment: MozReview Request: Add OpenSans fonts for content

Review commit: https://reviewboard.mozilla.org/r/53702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53702/

Comment 33

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/51419/#review50436

This is ready for final review -- please let me know if you have more suggestions or changes?  If it's something small we should push and iterate.

Comment 34

[:glob ✱]

https://reviewboard.mozilla.org/r/53682/#review50516

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:37
(Diff revision 1)
> -    width: 56px;
> +      width: 56px;
> +      height: 56px;

the png you've included is 56x43, so this will result in distortion.

please update the logo to be double resolution , which will look much better on hidpi devices.

::: pylib/mozreview/mozreview/static/mozreview/js/default.js:9
(Diff revision 1)
>  
>    // Fix logout url
>    $("a[href='/account/logout/']").attr("href", "/mozreview/logout/");
>    $("#accountnav li").css("visibility", "visible");
>  
>    /* DW attempt at a redesign */

as per my previous review, this comment needs to be updated.

Comment 35

[David Walsh :davidwalsh]

Comment on attachment 8754059 [details]
MozReview Request: Add OpenSans fonts for content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53702/diff/1-2/

Comment 36

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/53682/#review50516

> the png you've included is 56x43, so this will result in distortion.
> 
> please update the logo to be double resolution , which will look much better on hidpi devices.

Updated with svg file from Martell.

> as per my previous review, this comment needs to be updated.

My bad, I removed that comment from the CSS file but not the JS. :(

Comment 37

[Steven MacLeod [:smacleod]]

Comment on attachment 8750448 [details]
MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

https://reviewboard.mozilla.org/r/51419/#review51856

I like the overall look, great job.

This would be *way* easier to review after a couple of changes:
1. For each commit, please upload screenshots to that review request so I can see visually what it changes exactly (and review it in RB)
2. Expand on the commit messages with more detail about what exactly is changing.
3. Break the font introduction commit out of the change which actually uses them.

It might also be helpful for maintenance reasons do but some delimiting comments around this style overriding stuff, as well as comments about what blocks of style apply to so we know where to check when we upgrade / where to fix.

Comment 38

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/51419/#review51868

Updated Screenshots!

Comment 39

[Steven MacLeod [:smacleod]]

Comment on attachment 8750448 [details]
MozReview Request: MozReview: Quick try at a 'redesign' (Bug 1056377). r?smacleod,glob,gps,mcote,salva

Clearing my flag until the commits are restructured

Comment 40

[David Walsh :davidwalsh]

Comment on attachment 8754046 [details]
MozReview Request: Re-skin MozReview (Bug 1056377). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53686/diff/1-2/

Comment 41

[David Walsh :davidwalsh]

Comment on attachment 8754046 [details]
MozReview Request: Re-skin MozReview (Bug 1056377). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53686/diff/2-3/

Comment 42

[David Walsh :davidwalsh]

Attachment: MozReview: Update header title for branding (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55888/

Comment 43

[David Walsh :davidwalsh]

Attachment: MozReview: Update base site colors and fonts (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55890/

Comment 44

[David Walsh :davidwalsh]

Attachment: MozReview: Update header and navigation styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55892/

Comment 45

[David Walsh :davidwalsh]

Attachment: MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55894/

Comment 46

[David Walsh :davidwalsh]

Attachment: MozReview: Update Diff Viewer and grid styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55896/

Comment 47

[David Walsh :davidwalsh]

Attachment: MozReview: Update dialog and comment dialog styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/55898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55898/

Comment 48

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55886/#review52616

Added screenshots to relevant reviews.  I know a few of the commits look big but they're 90% color changes or removal of border-radius.  Hopefully we can get this in and iterate where necessary.

Comment 49

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/55894/#review52678

Comment 50

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/55892/#review52682

Comment 51

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55892/#review52682

Comment 52

[David Walsh :davidwalsh]

Comment on attachment 8757457 [details]
MozReview: Update header title for branding (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55888/diff/1-2/

Comment 53

[David Walsh :davidwalsh]

Comment on attachment 8757458 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55890/diff/1-2/

Comment 54

[David Walsh :davidwalsh]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55892/diff/1-2/

Comment 55

[David Walsh :davidwalsh]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55894/diff/1-2/

Comment 56

[David Walsh :davidwalsh]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55896/diff/1-2/

Comment 57

[David Walsh :davidwalsh]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55898/diff/1-2/

Comment 58

[David Walsh :davidwalsh]

Comment on attachment 8757457 [details]
MozReview: Update header title for branding (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55888/diff/2-3/

Comment 59

[David Walsh :davidwalsh]

Comment on attachment 8757458 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55890/diff/2-3/

Comment 60

[David Walsh :davidwalsh]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55892/diff/2-3/

Comment 61

[David Walsh :davidwalsh]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55894/diff/2-3/

Comment 62

[David Walsh :davidwalsh]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55896/diff/2-3/

Comment 63

[David Walsh :davidwalsh]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55898/diff/2-3/

Comment 64

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/55894/#review52678

Comment 65

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55894/#review54850

Updated screenshot!

Comment 66

[Steven MacLeod [:smacleod]]

Comment on attachment 8757457 [details]
MozReview: Update header title for branding (Bug 1056377).

https://reviewboard.mozilla.org/r/55888/#review55890

::: pylib/mozreview/mozreview/static/mozreview/js/default.js:9
(Diff revision 3)
> +  // Update heading title for branding
> +  $('#headerbar #title').html('Review Board @ Mozilla');

I think it's valuable to keep the version information in the header (or at least somewhere) - I use it often to compare what version we're running with new releases. It's also useful to other users when viewing documentation as each RB version has its own set of docs with different features

::: pylib/mozreview/mozreview/static/mozreview/js/default.js:10
(Diff revision 3)
>    // Fix logout url
>    $("a[href='/account/logout/']").attr("href", "/mozreview/logout/");
>    $("#accountnav li").css("visibility", "visible");
> +
> +  // Update heading title for branding
> +  $('#headerbar #title').html('Review Board @ Mozilla');

Lets keep it so clicking this text links you to `/`.

Comment 67

[Steven MacLeod [:smacleod]]

Comment on attachment 8757458 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

https://reviewboard.mozilla.org/r/55890/#review55894

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:23
(Diff revision 3)
> +  border-radius: @arguments;
> +}
> +
> +@font-face {
> +    font-family: 'Open Sans';
> +    src: url('../fonts/OpenSans-Light-webfont.eot');

Since we're distributing these fonts we need to provide the license along with them.

::: pylib/mozreview/mozreview/static/mozreview/fonts/OpenSans-Light-webfont.svg:1
(Diff revision 3)
> +<?xml version="1.0" standalone="no"?>

How vital is this font change to the new look and feel? I'd love to see some screenshots / some justification for why we're introducing them.

What sort of impract on performance can we expect?

Comment 68

[Steven MacLeod [:smacleod]]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

https://reviewboard.mozilla.org/r/55892/#review55908

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:76
(Diff revision 3)
> +@media screen and (max-width: 720px), screen and (max-device-width: 720px) and (orientation: landscape) {
> +  #headerbar #rbinfo a {
> +    display: none;
> +  }
> +}

Can you please include a screenshot where this would apply?

Comment 69

[Steven MacLeod [:smacleod]]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

https://reviewboard.mozilla.org/r/55894/#review55916

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:146
(Diff revision 3)
> +/* Review Requests */
> +.review-request {

Anything that only applies to review request pages should be in the `review` bundle, rather than the `default` bundle which is applied to every page.

It might even make sense to put the reskin into its own less files (one in the `default` bundle and one in the `review`), they'll be minified into the same CSS file as the rest of the bundle, but gives us the separation.

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:207
(Diff revision 3)
> +
> +.actions .menu {
> +  border: 0;
> +}
> +
> +/* Review Banners */

Could you please include screenshots showing the updated review boxes and banners?

Comment 70

[Steven MacLeod [:smacleod]]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

https://reviewboard.mozilla.org/r/55896/#review55930

::: pylib/mozreview/mozreview/static/mozreview/css/common.less:321
(Diff revision 3)
> +    }
> +
> +
> +    .datagrid-title-tabs .datagrid-tab {
> +      border: 0;
> +      

trailing whitespace

Comment 71

[Steven MacLeod [:smacleod]]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

https://reviewboard.mozilla.org/r/55898/#review55938

Comment 72

[Barret Rennie [:barret] (they/them)]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

https://reviewboard.mozilla.org/r/55892/#review56630

Comment 73

[David Walsh :davidwalsh]

Comment on attachment 8757458 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55890/diff/3-4/

Comment 74

[David Walsh :davidwalsh]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55892/diff/3-4/

Comment 75

[David Walsh :davidwalsh]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55894/diff/3-4/

Comment 76

[David Walsh :davidwalsh]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55896/diff/3-4/

Comment 77

[David Walsh :davidwalsh]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55898/diff/3-4/

Comment 78

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55892/#review56630

Comment 79

[David Walsh :davidwalsh]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55892/diff/4-5/

Comment 80

[David Walsh :davidwalsh]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55894/diff/4-5/

Comment 81

[David Walsh :davidwalsh]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55896/diff/4-5/

Comment 82

[David Walsh :davidwalsh]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55898/diff/4-5/

Comment 83

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55890/#review55894

I've removed the special fonts.  I've been told by Martell that we're switching to a different standard Mozilla font so I'll let others do that first.

> Since we're distributing these fonts we need to provide the license along with them.

Dropping this; it's not a bad idea but neither MDN or Bedrock do this and so I don't think it should block.

> How vital is this font change to the new look and feel? I'd love to see some screenshots / some justification for why we're introducing them.
> 
> What sort of impract on performance can we expect?

There will be a slight performance hit because (1) it's a new resource and (2) there's font-rendering time.  I've updated to use this font because it's Mozilla's standard font, used on every Mozilla property.

Comment 84

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55892/#review55908

> Can you please include a screenshot where this would apply?

Added; the branding would take up valuable vertical real estate on small screens, and it's only for vanity, so we don't need it.

Comment 85

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55892/#review56630

Comment 86

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55896/#review55930

Comment 87

[David Walsh :davidwalsh]

Attachment: MozReview: Move new skin to its own files (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/60558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60558/

Comment 88

[David Walsh :davidwalsh]

Comment on attachment 8757458 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55890/diff/4-5/

Comment 89

[David Walsh :davidwalsh]

Comment on attachment 8757459 [details]
MozReview: Update header and navigation styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55892/diff/5-6/

Comment 90

[David Walsh :davidwalsh]

Comment on attachment 8757460 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55894/diff/5-6/

Comment 91

[David Walsh :davidwalsh]

Comment on attachment 8757461 [details]
MozReview:  Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55896/diff/5-6/

Comment 92

[David Walsh :davidwalsh]

Comment on attachment 8757462 [details]
MozReview:  Update dialog and comment dialog styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55898/diff/5-6/

Comment 93

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55894/#review55916

> Anything that only applies to review request pages should be in the `review` bundle, rather than the `default` bundle which is applied to every page.
> 
> It might even make sense to put the reskin into its own less files (one in the `default` bundle and one in the `review`), they'll be minified into the same CSS file as the rest of the bundle, but gives us the separation.

Somewhat addressed here:  https://reviewboard.mozilla.org/r/60558/

Comment 94

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/60558/#review57442

I've tried to logically move the skin stuff here.  Quick note:  this *isn't* working -- `./mozreview refresh` is bombing because it can't find the correct path to the `-vars.less` file, despite the first path being correct.

```
raise CompilerError(stderr)
pipeline.exceptions.CompilerError: FileError: 'mozilla-theme-vars.less' wasn't found. Tried - /version-control-tools/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-vars.less,/version-control-tools/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-vars.less,/version-control-tools/pylib/mozreview/mozreview/mozilla-theme-vars.less,/venv/lib/python2.6/site-packages/reviewboard/mozilla-theme-vars.less,/venv/lib/python2.6/site-packages/djblets/mozilla-theme-vars.less,mozilla-theme-vars.less in /version-control-tools/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-review.less on line 2, column 1:
1 @RB_MINOR_VERSION: 5;@STATIC_ROOT: "/static/";@DEBUG: false;@RB_IS_RELEASED: 0;@RB_MICRO_VERSION: 4;@RB_PATCH_VERSION: 0;@RB_MAJOR_VERSION: 2;
2 @import (reference) 'mozilla-theme-vars';
```

I've also pushed this for discussion of how we'd like to do this moving forward.  Any comments welcome.

Comment 95

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/60558/#review58454

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:1
(Diff revision 1)
> +@import 'mozilla-theme-vars.less';

Adding stub issue for overall error.

Comment 96

[David Walsh :davidwalsh]

Comment on attachment 8764987 [details]
MozReview: Move new skin to its own files (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60558/diff/1-2/

Comment 97

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/55886/#review58622

Closing this because it requires an update that should be made post-rbbz-rebase.  This review request got hosed.

Comment 98

[David Walsh :davidwalsh]

Attachment: MozReview: Update base site colors and fonts (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63520/

Comment 99

[David Walsh :davidwalsh]

Attachment: MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63522/

Comment 100

[David Walsh :davidwalsh]

Attachment: MozReview: Update Diff Viewer and grid styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63524/

Comment 101

[David Walsh :davidwalsh]

Attachment: MozReview: Update dialog and comment dialog styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63526/

Comment 102

[David Walsh :davidwalsh]

Attachment: MozReview: Move new skin to its own files (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63528/

Comment 103

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/63528/#review60388

Let me know if this is more to your liking.  I can redo the review structure and start the changes in this format.

Comment 104

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/63528/#review60388

Yup, separating it like that seems good to me :D

Comment 105

[David Walsh :davidwalsh]

Attachment: MozReview: Update base site colors and fonts (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63556/

Comment 106

[David Walsh :davidwalsh]

Attachment: MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63558/

Comment 107

[David Walsh :davidwalsh]

Attachment: MozReview: Update Diff Viewer and grid styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63560/

Comment 108

[David Walsh :davidwalsh]

Attachment: MozReview: Update dialog and comment dialog styles (Bug 1056377).

Review commit: https://reviewboard.mozilla.org/r/63562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63562/

Comment 109

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/63562/#review60998

Screenshots added

Comment 110

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8769858 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

https://reviewboard.mozilla.org/r/63556/#review61008

The code looks fine, and the screenshots are baller.

Comment 111

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8769859 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

https://reviewboard.mozilla.org/r/63558/#review61010

Just a quick question about border-radius(0);. Otherwise, lgtm.

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less:8
(Diff revision 1)
> +/* Review Requests */
> +.review-request {
> +  .main {
> +    background: #fff;
> +    border: 0;
> +    border-radius: 0;

Woo - no border-radius!

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less:28
(Diff revision 1)
> +  background: @moz-blue-dark;
> +  border: 0;
> +}
> +
> +.actions-container {
> +  .border-radius(0);

At first I thought this was an error, and then I realized that LESS allows us to [define macros like this](https://github.com/reviewboard/reviewboard/blob/1ddc83098007246c1790d7a1cd9b2c4cdb4713c0/reviewboard/static/rb/css/mixins/style.less#L6)

According to the documentation in that file, these macros are deprecated. Are we able to just use `border-radius: 0;` directly?

Same for other uses in this file.

Comment 112

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8769860 [details]
MozReview: Update Diff Viewer and grid styles (Bug 1056377).

https://reviewboard.mozilla.org/r/63560/#review61012

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:129
(Diff revision 1)
> +  #page_sidebar {
> +    .page-sidebar-items {
> +      li.has-url {
> +        &:hover {
> +          .page-sidebar-row {
> +            .border-radius(0);

Same as other commit regarding this macro, including other uses in this commit.

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:130
(Diff revision 1)
> +    .page-sidebar-items {
> +      li.has-url {
> +        &:hover {
> +          .page-sidebar-row {
> +            .border-radius(0);
> +            background: rgba(255, 255, 255, 0.6);

Worth putting into a variable, maybe in a follow-up?

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:163
(Diff revision 1)
> +
> +  .datagrid-header {
> +    background: @moz-blue-dark;
> +  }
> +}
> +body.datagrid-page {

Nit: newline before this new block please.

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:218
(Diff revision 1)
> +
> +    .datagrid-title-tabs .datagrid-tab {
> +      border: 0;
> +
> +      &.active {
> +        background: #d4dde4;

Worth extracting this out to a variable, maybe in a follow-up patch?

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:253
(Diff revision 1)
> +	border-left: 1px solid @moz-blue-light;
> +	padding: 0 20px;

Busted indentation

Comment 113

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8769861 [details]
MozReview: Update dialog and comment dialog styles (Bug 1056377).

https://reviewboard.mozilla.org/r/63562/#review61014

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:276
(Diff revision 1)
>  }
> +
> +/* Dialogs */
> +.modalbox-inner, .modalbox-title, .modalbox {
> +  background-image: none;
> +  .border-radius(0);

Same as before with this macro.

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:281
(Diff revision 1)
> +  .border-radius(0);
> +}
> +
> +.modalbox-title {
> +  background: @moz-blue-dark;
> +  border-bottom-color: #888a85;

Variable for this?

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:289
(Diff revision 1)
> +.modalbox-inner {
> +  background: @moz-blue-light;
> +}
> +
> +.modalbox-bg {
> +  background: rgba(255, 255, 255, 0.6) !important; /* overriding JS */

And this too?

Comment 114

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/63558/#review61010

> At first I thought this was an error, and then I realized that LESS allows us to [define macros like this](https://github.com/reviewboard/reviewboard/blob/1ddc83098007246c1790d7a1cd9b2c4cdb4713c0/reviewboard/static/rb/css/mixins/style.less#L6)
> 
> According to the documentation in that file, these macros are deprecated. Are we able to just use `border-radius: 0;` directly?
> 
> Same for other uses in this file.

I just realized you defined the macro yourself in the first patch - my bad for missing that.

Comment 115

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/63560/#review61012

> Same as other commit regarding this macro, including other uses in this commit.

Dropping, since you define this macro yourself in the first commit.

Comment 116

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/63562/#review61014

> Same as before with this macro.

Dropping, since you define this macro yourself in the first commit.

Comment 117

[David Walsh :davidwalsh]

Comment on attachment 8769858 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63556/diff/1-2/

Comment 118

[David Walsh :davidwalsh]

Comment on attachment 8769859 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63558/diff/1-2/

Comment 119

[David Walsh :davidwalsh]

Comment on attachment 8769860 [details]
MozReview: Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63560/diff/1-2/

Comment 120

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/63560/#review61034

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:282
(Diff revision 2)
> +  .border-radius(0);
> +}
> +
> +.modalbox-title {
> +  background: @moz-blue-dark;
> +  border-bottom-color: #888a85;

Didn't add a var for this because this dialog will be going away shortly with the dropdown dialog; color is the current dialog border color.

Comment 121

[Steven MacLeod [:smacleod]]

Comment on attachment 8769858 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

https://reviewboard.mozilla.org/r/63556/#review62676

Comment 122

[Steven MacLeod [:smacleod]]

Comment on attachment 8769859 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

https://reviewboard.mozilla.org/r/63558/#review62678

Comment 123

[Steven MacLeod [:smacleod]]

Comment on attachment 8769860 [details]
MozReview: Update Diff Viewer and grid styles (Bug 1056377).

https://reviewboard.mozilla.org/r/63560/#review62674

Looks great :D, just the little overflow issue.

Comment 124

[David Walsh :davidwalsh]

Comment on attachment 8769858 [details]
MozReview: Update base site colors and fonts (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63556/diff/2-3/

Comment 125

[David Walsh :davidwalsh]

Comment on attachment 8769859 [details]
MozReview: Update Review Request page, banner, and comments styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63558/diff/2-3/

Comment 126

[David Walsh :davidwalsh]

Comment on attachment 8769860 [details]
MozReview: Update Diff Viewer and grid styles (Bug 1056377).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63560/diff/2-3/

Comment 127

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/63560/#review62674

Comment 128

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/58ad2f8e01e1
MozReview: Update base site colors and fonts . r=mconley,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/20e40a4963c0
MozReview: Update Review Request page, banner, and comments styles . r=mconley,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/77d80b0b77e5
MozReview: Update Diff Viewer and grid styles . r=mconley,smacleod

Comment 129

[Gregory Szorc [:gps]]

The prod deploy didn't like the logo.svg change.

It was complaining during "collectstatic" that a ext/mozreview.extension/MozReviewExtension/mozreview/logo.svg could not be found.

I hacked around this by copying the logo.svg into that directory on the admin server to unblock the deploy.

Comment 130

[David Walsh :davidwalsh]

Reopening in hopes it frees up to add a review request on MozReview...

Comment 131

[David Walsh :davidwalsh]

Keeping myself as needinfo for purpose of SVG issue if it pops up again.

Comment 132

[:glob ✱]

please don't reopen deployed bugs; file new bugs for issues.