Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from

Conversation

mythmon
Copy link
Contributor

@mythmon mythmon commented Mar 22, 2013

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?

This replaced Highcharts on the "Show Helpful Votes Chart" link in the
history of a KB article.
@mythmon
Copy link
Contributor Author

mythmon commented Mar 22, 2013

Bah. tests. I'll fix the tests.

<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l10n gettext() missing

@mythmon
Copy link
Contributor Author

mythmon commented Mar 25, 2013

Tests and L10n^.

@@ -497,6 +489,7 @@ article {
padding: 0 10px 0 0;
text-align: right;
}
display: none;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems out of place.

@rlr
Copy link
Contributor

rlr commented Mar 25, 2013

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

chart.series[0].hide();
chart.series[1].hide();

window.graphObjects = graphObjects;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for debugging?

@rlr
Copy link
Contributor

rlr commented Mar 25, 2013

(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
Copy link
Contributor Author

mythmon commented Mar 25, 2013

Feedback ^

@mythmon
Copy link
Contributor Author

mythmon commented Mar 25, 2013

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

@rlr
Copy link
Contributor

rlr commented Mar 25, 2013

Oh, that makes sense. Awesome!

@rlr
Copy link
Contributor

rlr commented Mar 25, 2013

WFM, r+ :shipit:

@mythmon
Copy link
Contributor Author

mythmon commented Mar 25, 2013

7bf4ac9

@mythmon mythmon closed this Mar 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants