Closed Bug 1077988 Opened 6 years ago Closed 6 years ago

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

Categories

(Webtools Graveyard :: Elmo, defect, P2)

Product:
Webtools Graveyard
Component:
Elmo
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Pike, Assigned: Pike)

Details

Attachments

(1 file, 1 obsolete file)

updated patch
96.43 KB, patch
adrian
: review+
Details | Diff | Splinter Review 
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.
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.
Attached patch all in one patch for my branch (obsolete) — — Splinter Review 
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.
Attachment #8503129 - Flags: review?(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?
Flags: needinfo?(l10n)
That sounds like webby. And I notice that my branch takes off before your full removal of webby.

Strange, shouldn't affect anything, though.
Flags: needinfo?(l10n)
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 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.
Attachment #8503129 - Flags: review?(adrian) → review-
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.
Attached patch updated patch — — Splinter Review 
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)
Attachment #8503129 - Attachment is obsolete: true
Attachment #8524102 - Flags: review?(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+
Attachment #8524102 - Flags: review?(adrian) → review+
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.
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.
This is also in prod, marking FIXED.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.