139.08 KB, image/png
198.53 KB, image/png
117.17 KB, image/png
4.81 KB, patch
|Details | Diff | Splinter Review|
We'd like to understand better how many locales are good and how many are lagging. To make that easier, we'd rather not count ourselves but have elmo do that. First bucket is current, second number is on the previous release, third bucket is "older than that". Mockup could be like: 20 | ab-CD | 1 And then a row group after that for the old bucket and the current bucket (which might span multiple beta milestones) Giving this a P2, as it'd be nice to have that soon to drill down our goals for next quarter.
Pardon my goldfish-like memory, but what's the "locale-changes page"? I wouldn't mind working on this but I think I'd need a refresher about what it is and how it would work.
https://l10n.mozilla.org/shipping/app/locale-changes/fx25 is an example URL, it's in apps/shipping/views/app.py
Wow! I started reading the code and my head exploded. In a bad way :) I think we better step through this bug in the next Elmo call. Then you can explain how you want this to work.
Created attachment 8338095 [details] Screen Shot 2013-11-25 at 3.24.05 PM.png The code I had to write to get to this is pretty horrible so I won't share yet but looking at the output (and pausing on aesthetics for the moment too) is this what you'd expect? I reversed the list of rows, and put things in buckets if the code name was the same (this puts fx23_beta_1 together with fx23) Then as I loop through all the rows, just before I dump a lump into a bucket I calculate the rowspan (and count the number of unique locales) and put that into the "last" row so it becomes easy to render in the template. What stumped me big time was that you said you wanted 4 groups and the third group should be fx20 and fx21. That I have not been able to achieve. I don't think I understand the data well enough to be able to make those groups.
Attachment #8338095 - Flags: feedback?(l10n)
Created attachment 8341959 [details] Screenshot 2013-12-03 16.43.45.png This way, the last of the 4 groups' rowspan is top vertical aligned. Now I just need to write a unit test to back this all up.
screenshot looks like what we need.
Created attachment 8342475 [details] [diff] [review] bug853068-2.diff I tried to start writing a test to prove this but there's nothing to build on for a test. I got started but quickly realised I would have to make fixture for every model under the sun. It just got messier and messier so in favor of progress some time this year I chickened out on a working test. The code is still a bit of a blur to me how it works with all the optimized dicts and lists of IDs. Perhaps it could be a follow-up bug to rewrite this into something more structured and modular.
Attachment #8342475 - Flags: review?(l10n)
Comment on attachment 8342475 [details] [diff] [review] bug853068-2.diff Review of attachment 8342475 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this actually does what we talked about. The groups were: This cycle Last cycle Prior two cycles Old cycles I find the code hard to grok as well, it'd be great if there were more comments on what that code actually does. Lastly, I'd prefer to go for classes for the rowspan rather than inline style, where possible. I'm OK with chickening out of a test for this, though.
Attachment #8342475 - Flags: review?(l10n) → review-
Comment on attachment 8338095 [details] Screen Shot 2013-11-25 at 3.24.05 PM.png This one showed the right numbers, as there's no Firefox 20. The only request I had for this was to align the last bucket top.
Attachment #8338095 - Flags: feedback?(l10n) → feedback+
The code under review lumps (aka. buckets) like this (from the bottom): 1. Latest release 2. Second latest release 3. Third latest release 4. All other older releases I believe you said you wanted it as: 1. Latest release 2. Second latest release 3. Third AND forth latest release 4. All other older releases This was something I could NOT make happen with the code. If you can please have a go :)
I think if you hook up the group logic inside the loop in line 58 while av_ and locs4av: it should be OK to get the same offset in releases grouped together. Probably keeping a counter there, storing the offset in rows, and inc'ing by len(accepted4loc), and on the next group bump, add the extra data to the row you kept. You can also ignore dealing with the milestone rows. There's a bit of paperwork, but I don't think it'll be worse than the code you're having.
I've taken this branch and rewrote the logic to support variable-sized buckets. The result is in https://github.com/Pike/elmo/compare/mozilla:develop...feature%2Fbug-853068-group-locale-changes?expand=1. There's one caveat, Peter, can you look at that? I can't figure out why the rowspan="2" doesn't open up the last column, there's still a full-width border between the two rows. The larger first and last buckets work fine and block out the border from the last column.
I tested it locally. It seems to work like a charm! You're good at this kind of stuff. How do you want to proceed? That you make a patch for final review or that I take your fork and make a patch for final review? After all, now it's almost entirely your code.
I'd appreciated if you could figure out the remaining html table problem here.
Created attachment 8374957 [details] show the table color, red square content bothers me Here's a screenshot to show where the table border is that bothers me, I squared it in red. The 8 actually counts 'nso te' and 'af bn-BD fa kk mk vi', aka, 18 and 19. But there's a border that looks like it's only counting 19.
Created attachment 8375037 [details] [diff] [review] bug853068-3.diff Your work, my css hack :)
Attachment #8375037 - Flags: review?(l10n) → review+
Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/90de3e64708a7058e958bd0ef4924381e5c20c56 bug 853068 - add counts to the locale-changes page to indicate current, recent, and old locales, r=Pike
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.