1289484 - Editing reviewer doesn't really let you see what you're typing

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: User Interface, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Screenshot showing the problem

I went to change the reviewer on a mozreview review request from me to someone else.  I clicked the little pencil image, and got the UI shown in the screenshot I'm attaching to this bug.

It's quite difficult to use a textfield if you can't tell what you're typing into it.  Especially if it comes prefilled with text (which you can't see).

Oh, and there's tons of space off to the right there that's not being used, so there's really no reason for the textfield to be this narrow.

I'm pretty sure the UI here used to be better.

Comment 1

[David Walsh :davidwalsh]

Attachment: MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 2

[David Walsh :davidwalsh]

Attachment: MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 3

[David Walsh :davidwalsh]

Attachment: MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 4

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 5

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 6

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 7

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 8

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 9

[:glob ✱]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

https://reviewboard.mozilla.org/r/68588/#review65846

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:49
(Diff revision 3)
>    border-top: 1px solid #C1C1C1;
>    border-right: 1px solid #C1C1C1;
>  
>    & > thead,
>    & > tbody {
> -    display:block;
> +    /* display:block; */

delete unnecessary code instead of commenting out

Comment 10

[:glob ✱]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

https://reviewboard.mozilla.org/r/68604/#review65856

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:74
(Diff revision 3)
> +        padding-right: 10px;
> +        margin-right: 10px;
>        }
>  
> -      .truncate_text {
> -        white-space: nowrap;
> +      a {
> +        color: #666;

i think these links should retain the link colouring.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:46
(Diff revision 3)
> +          <a class="mozreview_commit_summary" title="{{ child_details.summary}}" href="{{child_details.get_review_request.get_absolute_url}}">
> +            {{ child_details.summary }}
> +          </a>
> +          <ul>
> +            <li>
> -        <a href="{{child_details.get_review_request.repository.path}}/rev/{{child_details|commit_id|slice:":12"}}">
> +              <a href="{{child_details.get_review_request.repository.path}}/rev/{{child_details|commit_id|slice:":12"}}">

let's link to the actual raw diff - /raw-rev/ instead of /rev/

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:52
(Diff revision 3)
> -        </a>
> +              </a>
> -      </td>
> +            </li><li>
> -      <td class="diff">
> -        <a title="See diff for commit {{child_details|commit_id|slice:":12"}}"
> +              <a title="See diff for commit {{child_details|commit_id|slice:":12"}}"
> -           class="commit_sha" href="{{child_details.get_review_request.get_absolute_url}}diff/#index_header">
> +                 class="commit_sha" href="{{child_details.get_review_request.get_absolute_url}}diff/#index_header">
> -          {{child_details|commit_id|slice:":12"}}
> +                  {% trans "View Diff" %}

i love how this looks, but i'm not sure that the main link should be to the reviews/comments.

as a reviewer i think it's more important to link to the diff view.  this is especially important on new review requests, where the reviews tab is empty.

(i thought we had a bug for this, but i cannot find it).


perhaps we should make the primary link in the row navigate to the diff view, and add a link to reviews as the first item below it?

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:54
(Diff revision 3)
> +            </li><li class="diffstat">
> +              {{ child_details.get_review_request|diffstat_text:user }}
> +            </li>

this would benefit from a tooltip.  something like "Lines inserted/deleted"

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:65
(Diff revision 3)
> +
>        {% comment "TODO: show this column when the commit author will be available" %}
>        <td>{{ child_details.submitter }}</td>
>        {% endcomment %}
> +
>        <td class="reviewers">

these changes look fantastic, but don't actually fix the issue stated by this bug - when resizing the window it's possible for the reviewers column to be to narrow to support the edit controls.

Comment 11

[:glob ✱]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

https://reviewboard.mozilla.org/r/68624/#review65854

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:80
(Diff revision 2)
> -        <a class="issue-count" href="{{child_details.get_review_request.get_absolute_url}}#issue-summary" title="{{child_details.get_review_request.issue_open_count}} open issues">
> -          <span class="issue-icon">!</span>{{child_details.get_review_request.issue_open_count}}
> +      <td class="status approval-issues" title="{{child_details.get_review_request.issue_open_count}} open issues">
> +        <a class="issue-count" href="{{child_details.get_review_request.get_absolute_url}}#issue-summary">
> +          ! {{child_details.get_review_request.issue_open_count}}
>          </a>
> +      </td>
> -        {% elif child_details.get_review_request.approved %}
> +      {% elif child_details.get_review_request.approved %}
> -        <div class="approval" title="Approved For Landing - You have at least one valid ship it!">r+</div>
> +      <td class="status approval" title="Approved For Landing - You have at least one valid ship it!">
> +        r+
> +      </td>
> -        {% else %}
> +      {% else %}
> -        <div class="no-approval" title="{{child_details.get_review_request.approval_failure}}">r?</div>
> -        {% endif %}
> +      <td class="status no-approval" title="{{child_details.get_review_request.approval_failure}}">
> +        r?
>        </td>

since you're touching this code, can you please fix the indentation -- the html between the {% %} lines should be indented one more level

Comment 12

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 13

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 14

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 15

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/68624/#review66002

Comment 16

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 17

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 18

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 19

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 20

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 21

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 22

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/68604/#review65856

> i think these links should retain the link colouring.

I disagree -- I think having them blue, along with the bold link above and the red/green color on the right, would make the cell look a birage of colors.  Since the diff link (most will use the "Diff" tab on the top right) and HG link (we said yesterday it was almost useless) aren't going to be clicked nearly as much as the others, I think keeping it toned down will be more eye-pleasing.

> i love how this looks, but i'm not sure that the main link should be to the reviews/comments.
> 
> as a reviewer i think it's more important to link to the diff view.  this is especially important on new review requests, where the reviews tab is empty.
> 
> (i thought we had a bug for this, but i cannot find it).
> 
> 
> perhaps we should make the primary link in the row navigate to the diff view, and add a link to reviews as the first item below it?

I think that could be good moving forward but maybe we do that outside of this set?  :smacleod - do you agree?

Comment 23

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/68604/#review65856

> these changes look fantastic, but don't actually fix the issue stated by this bug - when resizing the window it's possible for the reviewers column to be to narrow to support the edit controls.

I added a bit to have a minimum width :)

Comment 24

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/68604/#review65856

> I disagree -- I think having them blue, along with the bold link above and the red/green color on the right, would make the cell look a birage of colors.  Since the diff link (most will use the "Diff" tab on the top right) and HG link (we said yesterday it was almost useless) aren't going to be clicked nearly as much as the others, I think keeping it toned down will be more eye-pleasing.

Maybe they should be blue but use a lighter font-weight (I will say, I'm not *too* bothered by `#666`, but they could feel a little more linky)? Aside: should we really leave the underline on the commit description text?

> I think that could be good moving forward but maybe we do that outside of this set?  :smacleod - do you agree?

I think maybe we should just do it in one go here as :glob is suggesting. My reasoning would be so that this link location change comes along with the larger restructuring, rather than swapping things out on people once they're used to the table change we're making.

Comment 25

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

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

Comment 26

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

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

Comment 27

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 28

[:glob ✱]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

https://reviewboard.mozilla.org/r/68588/#review66310

Comment 29

[:glob ✱]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

https://reviewboard.mozilla.org/r/68604/#review66312

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:154
(Diff revision 7)
> -     border-left: 1px solid #C1C1C1;
> +   border-left: 1px solid #C1C1C1;
> -     border-bottom: 1px solid #C1C1C1;
> +   border-bottom: 1px solid #C1C1C1;
> -   }
> + }
>  
> -   & > thead > tr {
> + & > thead > tr {
> -     background: transparent url("../header_bg.png") repeat-x scroll left bottom;
> +   background: @moz-blue-dark;

NameError: variable @moz-blue-dark is undefined

you need to @import mozilla-theme-common.less

Comment 30

[David Walsh :davidwalsh]

Comment on attachment 8776973 [details]
MozReview: Update commits table layout for display on small screens (Bug 1289484).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68588/diff/7-8/

Comment 31

[David Walsh :davidwalsh]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68604/diff/7-8/

Comment 32

[David Walsh :davidwalsh]

Comment on attachment 8777018 [details]
MozReview: Make the status column more pleasing to the eye (Bug 1289484).

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

Comment 33

[:glob ✱]

Comment on attachment 8776992 [details]
MozReview: Update format of commits table to provide more space for important pieces (Bug 1289484).

https://reviewboard.mozilla.org/r/68604/#review66604

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/706dfc0da104
MozReview: Update commits table layout for display on small screens . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b2bc69b3c237
MozReview: Update format of commits table to provide more space for important pieces . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3da72b32267d
MozReview: Make the status column more pleasing to the eye . r=glob