1077988 - [homepage][shipping] make team page smaller, action and status more useful

Status: RESOLVED FIXED

Description

[Axel Hecht [:Pike]]

I've run another attack on the team page.

I've seriously condensed the vertical screen usage, by pulling the product into the table rows, but I also reduced the space usage in the rows themselves.

While doing so, I recognized that the status isn't decipherable at all, really, and so I split this up into a column with status, and a column with actions. Those are now ordered by time.

https://github.com/Pike/elmo/compare/feature/smaller-team-page is my branch, anyone volunteering to review the beast?

I did run through all team pages on two data dumps, a few times. So there's little risk that the data is breaking, or that it's showing wrong stuff. I also got feedback from Matjaz and flod on in-progress screenshots.

Comment 1

[Stefan Plewako [:stef]]

Progress graphs are hard to understand and thus useless (at least for me).

All in all it still looks huge and I see lots of whitespace, maybe not gigantic like now but still far far away from normal and way too much scrolling to use it instead shipping dashboard.

Comment 2

[Axel Hecht [:Pike]]

Attachment: all in one patch for my branch

I'm updating the branch at https://github.com/Pike/elmo/compare/mozilla:develop...feature/smaller-team-page, but here's a full blown patch (without the binaries) for review.

Comment 3

[Adrian Gaudebert [:adrian]]

I am getting the following error when testing from your branch: http://pastebin.mozilla.org/6797987

Doesn't happen on develop of course, but I can't figure out what in your changes might cause it. Do I need to do something special to make it work?

Comment 4

[Axel Hecht [:Pike]]

That sounds like webby. And I notice that my branch takes off before your full removal of webby.

Strange, shouldn't affect anything, though.

Comment 5

[Adrian Gaudebert [:adrian]]

Comment on attachment 8503129 [details] [diff] [review]
all in one patch for my branch

Review of attachment 8503129 [details] [diff] [review]:
-----------------------------------------------------------------

I like the look of it overall! 

 * I would add a little explanation to the icons, like in a title attribute. Especially those in the Sign-offs column, there's at least one that I have no clue about (the green one that looks like a graph). 
 * I would also remove the color change (white / grey / white... ) on the title cell (Firefox, Fennec... ) and let them all be white
 * Some HTML files are mixing 2-spaces and 4-spaces indentation. Shouldn't we stick to one pattern? 
 * Feel free to disregard the styling comments.

::: apps/shipping/templates/shipping/team-snippet.html
@@ +14,5 @@
>                <th>Tree</th>
>                <th>Translation Status</th>
>                <th>Progress</th>
> +              <th>Sign-offs </th>
> +              <th>Actions <sup><a href="https://developer.mozilla.org/docs/Mozilla/Localization/Sign-off_reviews" title="What is this?">?</a></sup></th>

I don't like the title of that link, I'd rather have a text that tells me what will happen when I click the button, or a short help with "click for more" for example. Right now, "What is this?" doesn't help me at all. :)

@@ +63,5 @@
>                <div class="treeprogress-graph" style="background-position:{{run.prog_pos.x}}px {{run.prog_pos.y}}px;"></div>
>              </a>
>              {% endif %}
>            </td>
> +          <td class="signoffs status{% if not run.fallback %} OK{% endif %} {{ runcycle }}">

Nit: it's weird to see a CSS class with upper-case.

@@ +68,4 @@
>                <!-- Accepted -->
>                {% if run.accepted %}
> +                     {% if run.fallback %}
> +                     <span class="oi status-fallback" data-glyph="warning"></span> {{ run.fallback }}

``oi`` doesn't make sense to me, I thus think it's a bad class name. :)

@@ +92,5 @@
> +            </a>
> +        {% else %}
> +            {% for action in run.actions %}
> +              <a class="button button-white {{ action.flag_name }}" href="{% url 'shipping.views.signoff.signoff' run.locale.code run.appversion.code %}#{{ action.rev }}">
> +                  {% if action.flag_name = "pending" %}

An equal char is missing here.

::: apps/shipping/views/__init__.py
@@ +178,4 @@
>                        run.suggested_shortrev = \
> +                      run.is_active = run.under_review = \
> +                      run.suggest_glyph = run.suggest_class = \
> +                      run.fallback = None

I find this hard to read and error-prone. How about something like this?

defaults = (
    'pending', 'actions', 'accepted', 'suggested_shortrev', 
    'suggested_shortrev', 'fallback', 'appversion', 'is_active', 
    'under_review', 'suggest_glyph', 'suggest_class', 'fallback',
)
for attr in defaults:
    setattr(run, attr, None)

@@ +203,5 @@
> +                    .select_related('signoff__push')
> +                    .order_by('when'))
> +                # only keep a rejected sign-off it's the last
> +                if Action.REJECTED in flags and \
> +                    actions[-1].flag != Action.REJECTED:

Another styling preference, but I like parenthesis better in `if` statements. For example, I find find this more readable: 

if (
    Action.REJECTED in flags and
    actions[-1].flag != Action.REJECTED
):
    # Things

@@ +252,5 @@
>      for runs in applications.itervalues():
>          for run in runs:
> +            actions = [run.accepted] if run.accepted else []
> +            if run.actions:
> +                actions += run.actions

If I'm correct, that means you could have the same action twice in ``actions``. Is that fine?

Comment 6

[Adrian Gaudebert [:adrian]]

Comment on attachment 8503129 [details] [diff] [review]
all in one patch for my branch

Review of attachment 8503129 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of some small issues that need to be fixed.

Comment 7

[Axel Hecht [:Pike]]

I'm updating the patch queue right now, there's a few things I did, and a few things I didn't.

The actions piece is deduplicated here: https://github.com/Pike/elmo/blob/daecb668830fc5db6c9f74061cd53b8567748d52/apps/shipping/views/__init__.py#L224

I moved to a slightly different layout of the table, http://i.imgur.com/esVcx6z.png, with just td borders, and the separator has no more borders. Instead, the product has border-radii, which enhance the grouping without adding too much visual noise.

The oi class names are part of the open-iconic library, so I kept those.

I agree that it'd be nice to have consistent styling in our html templates, but that should be a separate bug so that the actual bug is easy to read.

I'll push my current branch, and then rebase and squash it for review on a new branch.

Comment 8

[Axel Hecht [:Pike]]

Attachment: updated patch

Updated patch.

https://github.com/Pike/elmo/tree/feature/smaller-team-page is forwarded, and https://github.com/Pike/elmo/tree/feature/bug-1077988-smaller-team-page is the single rebased patch. (They're the same tree but a single whitespace change)

Comment 9

[Adrian Gaudebert [:adrian]]

Comment on attachment 8524102 [details] [diff] [review]
updated patch

Review of attachment 8524102 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me! The only comment I have is that I would add the help link next to the Sign-offs header as well. 

r+

Comment 10

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/3bb9baab96ba767d949faee233684070487e25c9
bug 1077988, make team page more compact, and sign-off info more useful, r=adrian.

Compact the team page by moving the product into a column, and generally
reduce line heights.

This patch also reworks how we're showing Sign-Offs and the l10n progress
around those.
Actions are now shown in chronological order, and there's a status indicator
column that presents statuses like good, up-to-date with missing strings,
etc.

Comment 11

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/705f8831da1949bedc2f659371167724c7bf3323
bug 1077988, make product names a tad smaller
Making the appnames be h3 instead of h2 is also better semantically.

Comment 12

[Axel Hecht [:Pike]]

This is also in prod, marking FIXED.

Comment 13

[[github robot]]

Commits pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/ed9bd9a6681aa3855d4124506040dc6ac380c4f7
bug 1077988, anchors regressed, fix that

https://github.com/mozilla/elmo/commit/fddda1c12e4adfb453391c8f5aaa3dd510f5584d
Merge hotfix for bug 1077988: anchor regression

Comment 14

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/69a4d92157fbdba818e678ca6fd61a3f6ef6cfa3
Merge hotfix for bug 1077988: anchor regression