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)
Webtools Graveyard
Elmo
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Pike, Assigned: Pike)
Details
Attachments
(1 file, 1 obsolete file)
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.
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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•6 years ago
|
||
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-
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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+
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 12•6 years ago
|
||
This is also in prod, marking FIXED.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 13•6 years ago
|
||
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•6 years ago
|
||
Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/69a4d92157fbdba818e678ca6fd61a3f6ef6cfa3 Merge hotfix for bug 1077988: anchor regression
Updated•4 months ago
|
Product: Webtools → Webtools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•