code review 6453062: misc/dashboard/codereview: recognize "NOT LGTM".

misc/dashboard/codereview: recognize "NOT LGTM".

A "NOT LGTM" overrules a previous "LGTM" by the same person, and vice versa.
"NOT LGTM"s are shown in the same location as LGTMs, colored red.

[dsymonds, 2012-07-30 00:19:24]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2012-07-30 00:21:05]

Let me know if a different shade would work for you better.

[rsc, 2012-07-30 00:49:30]

LGTM

I'm not sure any red is needed. There's so little color on the page,
and the NOT is pretty hard to miss. So I'd be inclined to just leave
them black like the LGTMs.

[dsymonds, 2012-07-30 00:53:18]

On Mon, Jul 30, 2012 at 10:49 AM, Russ Cox <rsc@golang.org> wrote:

> I'm not sure any red is needed. There's so little color on the page,
> and the NOT is pretty hard to miss. So I'd be inclined to just leave
> them black like the LGTMs.

I think it helps to have them pop a little, especially if a CL has
some LGTMs and some not LGTMs. A NOT LGTM is/should be fairly rare;
highlighting the exceptional is good design.

Let's see how it goes. We can always go back to black if it turns out
to be annoying.

[rsc, 2012-07-30 00:54:39]

LGTM

[dsymonds, 2012-07-30 00:54:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=5f7a13ebcd92 ***

misc/dashboard/codereview: recognize "NOT LGTM".

A "NOT LGTM" overrules a previous "LGTM" by the same person, and vice versa.
"NOT LGTM"s are shown in the same location as LGTMs, colored red.

R=rsc
CC=golang-dev
http://codereview.appspot.com/6453062

[minux1, 2012-07-30 00:59:12]

http://codereview.appspot.com/6453062/diff/3/misc/dashboard/codereview/dashbo...
File misc/dashboard/codereview/dashboard/front.go (right):

http://codereview.appspot.com/6453062/diff/3/misc/dashboard/codereview/dashbo...
misc/dashboard/codereview/dashboard/front.go:247: {{if and .LGTMs
$tbl.Assignable}}<br /><span style="font-size: smaller;">LGTMs:
{{.LGTMHTML}}{{end}}</span>
i think these </span> nested wrongly.

http://codereview.appspot.com/6453062/diff/3/misc/dashboard/codereview/dashbo...
misc/dashboard/codereview/dashboard/front.go:248: {{if and .NotLGTMs
$tbl.Assignable}}<br /><span style="font-size: smaller; color: #f74545;">NOT
LGTMs: {{.NotLGTMHTML}}{{end}}</span>
ditto

[dsymonds, 2012-07-30 01:10:59]

http://codereview.appspot.com/6453062/diff/3/misc/dashboard/codereview/dashbo...
File misc/dashboard/codereview/dashboard/front.go (right):

http://codereview.appspot.com/6453062/diff/3/misc/dashboard/codereview/dashbo...
misc/dashboard/codereview/dashboard/front.go:247: {{if and .LGTMs
$tbl.Assignable}}<br /><span style="font-size: smaller;">LGTMs:
{{.LGTMHTML}}{{end}}</span>
On 2012/07/30 00:59:12, minux wrote:
> i think these </span> nested wrongly.

Oops, you're right. I'll fix.