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

Replace charting library in admin #333

Merged
merged 5 commits into from
Feb 5, 2017
Merged

Replace charting library in admin #333

merged 5 commits into from
Feb 5, 2017

Conversation

GUI
Copy link
Member

@GUI GUI commented Feb 5, 2017

This replaces our usage of Google Charts within the admin with ECharts. While not really necessary and not the highest priority, a few things led to this:

  • Testing: Something about Google Charts seemed to lead to random issues in our Capybara integration tests. While these mostly seemed like false positives due to weird interactions between PhantomJS and Google Charts loading in the background, causing intermittent errors in our test suite was annoying. I tried various workarounds, but nothing really seemed to work (I tried stubbing Google Charts to effectively disable it, which did seem to fix the test issues, but due to how it got stubbed, it required other workarounds for production).
  • Works without external internet access: When implementing local admin accounts (Local admin accounts #332), I was reminded that there's been a few requests over the years for API Umbrella to work in firewalled environments where access to things like Google may not be possible. With local admin accounts implemented, our only other tie to external internet were the charts and graphs. And while minor, I've also been bit by this a few times when trying to work on API Umbrella without internet access (eg, on a plane).
  • Features and performance: ECharts does have a number of nice features beyond what Google Charts provides. For example having chart zooming and timelines on all line chart types is nice (there's some other nice features too). Plus, some of the things ECharts does with rendering and sampling large datasets could be nice for performance if we want to increase the amount of data we display (for example, allowing more minutely data).

Overall, there wasn't actually a lot of changes required code-wise to make this switch. It was mostly a matter of changing the chart initialization options for EChart's syntax and options (but the chart types are actually pretty similar). A few other notes:

  • This actually fixes some existing issues with the charts on master, since there were some bugs after the Ember 2 upgrade (some things were still referencing defunct Ember controllers which had been removed).
  • The primary downside and complexity is probably related to the geographic maps. While ECharts provides a similar mapping library to Google Charts, ECharts doesn't provide the actual mapping data (country boundaries, state boundaries, etc). So this means we need to add this boundary data to our repo. I couldn't find any good, existing GeoJSON sources of this data that met our requirements (namely, aligning country abbreviations with MaxMind's GeoIP list of countries and having simplified geometries for small downloads/quick rendering). So I ended up creating a rake task to generate the needed boundary data based on Natural Earth Data's standard data sets. So while I'm not wild about this additional bit of complexity and needing to store this data, it should hopefully be more of a one-time issue that we won't have to touch again.

GUI added 5 commits January 22, 2017 22:20
This allows for the admin to be used offline and doesn't require an
external connection to Google (this also simplifies some of our testing
setup).
Something about setting styles on the element the Capybara environment
didn't like (said it was a readonly attribute). Not sure why this wasn't
happening in a real browser, but in any case, using CSS is better.
@GUI GUI merged commit 10140f6 into master Feb 5, 2017
@GUI GUI deleted the echarts branch February 5, 2017 15:29
@GUI GUI added this to the v0.14.0 milestone Feb 6, 2017
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.

1 participant