773478 - [l10nstats] switch from using simile timeplot to d3 for tree and locale history

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Axel Hecht [:Pike]]

Right now, we're using two different UIs and UXes based on simile timeplot for showing the history over time for a tree and an individual locale.

timeplot isn't maintained no more, and we should have a common UX about how to navigate in time.

I have a patch in the works for converting to d3.js.

Comment 1

[Axel Hecht [:Pike]]

Attachment: use same logic for tree and locale history, based on d3.js

These graphs are pretty feature compatible to what we had, aside from the follow-up for the change markers on the tree progress that I already filed.

I cleaned up the code a bit, and even addressed the last regression in the histogram (the cell-padding didn't work at all).

jshint passes on the files I changed, as well as check.py. I didn't jshint clusterer.js, or other files, gonna file a bug on those.

I also haven't looked yet at possible transitions when modifying the d3.js graphs, like, one could probably fade out and zoom when hiding the bad builds, and all that jazz that d3.js supports.

But none of that blocks getting off of the timeplot code.

Comment 3

[Peter Bengtsson [:peterbe]]

Comment on attachment 641878 [details] [diff] [review]
use same logic for tree and locale history, based on d3.js

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

r+ with biggest nit being on the repeated template code across history.html and tree_progress.html

::: apps/l10nstats/static/l10nstats/css/history.css
@@ +9,5 @@
> +}
> +
> +.marker.missing {
> +  stroke: red;
> +}

petty but... didn't we have a "standard" of 4 spaces in CSS?

::: apps/l10nstats/static/l10nstats/js/history.js
@@ +5,1 @@
>  function onLoad() {

I know this is unchanged code but that's a very poor function name.

::: apps/l10nstats/static/l10nstats/js/tree_progress.js
@@ +190,3 @@
>  
>    // histogram
>    var hist_div = document.getElementById('histogram');

Why exit the usefulness of jQuery here?

@@ +229,5 @@
>      hist_div = td.children()[0];
>      $(graphs_row).append(td);
> +    values = valuesForHist(hist);
> +    values.sort(numerical);
> +    _j = undefined;

This is strange code. Use `_j = null` to unset the variable. "undefined" is just weird. 

Also, instead of "_j" can we not call it "previous_j"?

::: apps/l10nstats/templates/l10nstats/history.html
@@ +7,5 @@
>  {% load compress %}
>  {% block title_matter %}[{{ locale}}, {{ tree }}] l10n stats{% endblock %}
>  
>  {% block head_matter %}
> +<script src="{{ STATIC_URL }}js/d3/d3.v2.min.js"></script>

Why is it loaded in the head?
If there's a reason to break the convention, please include a comment.

Actually, in tree_progress.html it is loaded from the regular JS block.

::: apps/l10nstats/templates/l10nstats/tree_progress.html
@@ +20,5 @@
>  <script>
> +var startdate = new Date({{stamps.start}}*1000),
> +    enddate = new Date({{stamps.end}}*1000),
> +    fullrange = [new Date({{stamps.startrange}}*1000), new Date({{stamps.endrange}}*1000)];
> +var tree = "{{ tree }}";

This is now repeating itself in history.html
Consider moving it to a separate .html file and use `{% include ... %}`

::: apps/l10nstats/views.py
@@ +131,5 @@
> +            'run': r.id
> +        }
> +    ]
> +    stamps = {}
> +    stamps['start'] = int(calendar.timegm(starttime.timetuple()))

Out of curiosity, what is this? How is this different from
`int(starttime)`?

Comment 4

[[github robot]]

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

https://github.com/mozilla/elmo/commit/dc9e19c090bc6a2ca7168546caf17e27b60d6961
bug 773478, convert tree and locale history to a common d3.js timeplot, r=peterbe

Comment 5

[Axel Hecht [:Pike]]

(In reply to Peter Bengtsson [:peterbe] from comment #3)
> Comment on attachment 641878 [details] [diff] [review]
> use same logic for tree and locale history, based on d3.js
> 
> Review of attachment 641878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with biggest nit being on the repeated template code across history.html
> and tree_progress.html
> 
> ::: apps/l10nstats/static/l10nstats/css/history.css
> @@ +9,5 @@
> > +}
> > +
> > +.marker.missing {
> > +  stroke: red;
> > +}
> 
> petty but... didn't we have a "standard" of 4 spaces in CSS?
> 
> ::: apps/l10nstats/static/l10nstats/js/history.js
> @@ +5,1 @@
> >  function onLoad() {
> 
> I know this is unchanged code but that's a very poor function name.

changed to renderGraph in both files

> ::: apps/l10nstats/static/l10nstats/js/tree_progress.js
> @@ +190,3 @@
> >  
> >    // histogram
> >    var hist_div = document.getElementById('histogram');
> 
> Why exit the usefulness of jQuery here?

I didn't really exit, I didn't start early. Changed much of the code to get the content creation into jquery.

> @@ +229,5 @@
> >      hist_div = td.children()[0];
> >      $(graphs_row).append(td);
> > +    values = valuesForHist(hist);
> > +    values.sort(numerical);
> > +    _j = undefined;
> 
> This is strange code. Use `_j = null` to unset the variable. "undefined" is
> just weird. 
> 
> Also, instead of "_j" can we not call it "previous_j"?

Done. I whacked this file pretty badly in the end to the content creation into jquery, and be less cryptic overall.

> ::: apps/l10nstats/templates/l10nstats/history.html
> @@ +7,5 @@
> >  {% load compress %}
> >  {% block title_matter %}[{{ locale}}, {{ tree }}] l10n stats{% endblock %}
> >  
> >  {% block head_matter %}
> > +<script src="{{ STATIC_URL }}js/d3/d3.v2.min.js"></script>
> 
> Why is it loaded in the head?
> If there's a reason to break the convention, please include a comment.
> 
> Actually, in tree_progress.html it is loaded from the regular JS block.

Done as part of the shared base template, see below.

> ::: apps/l10nstats/templates/l10nstats/tree_progress.html
> @@ +20,5 @@
> >  <script>
> > +var startdate = new Date({{stamps.start}}*1000),
> > +    enddate = new Date({{stamps.end}}*1000),
> > +    fullrange = [new Date({{stamps.startrange}}*1000), new Date({{stamps.endrange}}*1000)];
> > +var tree = "{{ tree }}";
> 
> This is now repeating itself in history.html
> Consider moving it to a separate .html file and use `{% include ... %}`

I've moved that code to a common base template, which is also helpful in creating other timeplot-like pages in the future.

> ::: apps/l10nstats/views.py
> @@ +131,5 @@
> > +            'run': r.id
> > +        }
> > +    ]
> > +    stamps = {}
> > +    stamps['start'] = int(calendar.timegm(starttime.timetuple()))
> 
> Out of curiosity, what is this? How is this different from
> `int(starttime)`?

It's different in the sense that it doesn't raise a TypeError ;-)

Marking FIXED.

Comment 6

[Peter Bengtsson [:peterbe]]

FYI, the simpler way to make a timestamp is:

>>> import time
>>> import datetime
>>> d = datetime.datetime.now()
>>> int(time.mktime(d.timetuple()))