Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[Bug 845775] Replace KB article history charts with Rickshaw #1225

Closed
wants to merge 6 commits into from
+4,263 −13,562

2 participants

@mythmon
Owner

s/highcharts/rickshaw/.

To test: Go to any KB article. Go to it's history. Click "Show Helpfulness Vote Chart." Play with it.

Of note:

  • Data is now grouped by week instead of by day.
  • No "minimap" below the scroll bar. This isn't built into Rickshaw, and I didn't want to build a new Rickshaw component for it.
  • The graph is technically a little less powerful, because of less UI elements, but I think it can do everything useful from the old version.
  • I also made some of the Rickshaw stuff I wrote before a little more reusable.

Apologies for the massive diff.

r?

mythmon added some commits
@mythmon
Owner

Bah. tests. I'll fix the tests.

apps/wiki/templates/wiki/history.html
@@ -25,7 +26,25 @@ <h1 class="title">{{ _('History of {title}')|fe(title=document.title) }}</h1>
</li>
</ul>
</div>
- <div id="helpful-chart" data-url="{{ url('wiki.get_helpful_votes_async', document.slug) }}"></div>
+
+ <div id="helpful-graph" data-url="{{ url('wiki.get_helpful_votes_async', document.slug) }}">
+ <div class="inline-controls">
+ <div>
+ <input type="radio" name="seriesset" value="percent">Percent</input>
@rlr Owner
rlr added a note

l10n gettext() missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mythmon
Owner

Tests and L10n^.

media/less/wiki.less
@@ -497,6 +489,7 @@ article {
padding: 0 10px 0 0;
text-align: right;
}
+display: none;
@rlr Owner
rlr added a note

This seems out of place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rlr
Owner

Is it easy to change the hover to something like "week of yyyy-mm-dd"?

media/js/historycharts.js
((271 lines not shown))
});
- chart.series[0].hide();
- chart.series[1].hide();
+
+ window.graphObjects = graphObjects;
@rlr Owner
rlr added a note

Is this for debugging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
media/js/historycharts.js
((232 lines not shown))
- show: function() {
- this.yAxis.axisTitle.show();
- }
- },
- dataGrouping: {
- enabled: false,
- }
- }
- },
- series: data
- }, function () {
- $('#show-chart').hide(); // loading complete callback
+ var lines = {};
+ var series = graphObjects.graph.series;
+ for (i=0; i<series.length; i++) {
+ var s = series[i];
@rlr Owner
rlr added a note

The end result isn't different, but it's better to declare the vars outside of the loop to avoid confusion. Same below around line 84.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rlr
Owner

(You probably know this) It looks like you can pass an xFormatter parameter to the HoverDetails thingy: https://github.com/shutterstock/rickshaw/blob/master/src/js/Rickshaw.Graph.HoverDetail.js#L9

Refactoring opportunities will be showing up quickly as we convert more graphs. I think this is a good first step though.

@mythmon
Owner

Feedback ^

@rlr

Wow, I didnt remember having k.dateFormat. nice

@mythmon
Owner

Actually, I added k.dateFormat because I wanted it in the second commit.

@rlr
Owner

Oh, that makes sense. Awesome!

@rlr
Owner

WFM, r+ :shipit:

@mythmon mythmon closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 22, 2013
  1. @mythmon

    Update Rickshaw's jQueryUI.

    mythmon authored
  2. @mythmon
  3. @mythmon

    [Bug 845775] Make KB helpful graphs use Rickshaw

    mythmon authored
    This replaced Highcharts on the "Show Helpful Votes Chart" link in the
    history of a KB article.
Commits on Mar 25, 2013
  1. @mythmon

    Fix tests.

    mythmon authored
  2. @mythmon

    L10n

    mythmon authored
  3. @mythmon
Something went wrong with that request. Please try again.