Remove a bit of unused code in multichart logic

woahdae commented 4 years ago

Commit 89534f7d9 removed some code without removing some dependent lines, which are now cruft. This removes those lines for clarity.

What does this PR do? How does it affect users?

Nothing, just refactoring.

Related tickets?

Nope

How should this be tested?

Unit tests?

Step through the code line by line. Things to keep in mind as you review:

Are there any edge cases not covered by this code? N/A
Does this code follow conventions (naming, formatting, modularization, etc) where applicable? Yes

Fetch the branch and/or deploy to staging to test the following:

[✓] Does the code compile without warnings (check shell, console)?
[✓] Do all tests pass?
[✓] Does the UI, pixel by pixel, look exactly as expected (check various screen sizes, including mobile)?
[N/A] If the feature makes requests from the browser, inspect them in the Web Inspector. Do they look as expected (parameters, headers, etc)?
[N/A] If the feature sends data to Keen, is the data visible in the project if you run an extraction (include link to collection/query)?
[N/A] If the feature saves data to a database, can you confirm the data is indeed created in the database?

maciejrybaniec commented 4 years ago

Hi @woahdae

Thank You for the contribution. We'll review this PR shortly. Also please take notice that we released a new version of Keen Dataviz with charts rebuild from scratch.

Here is an example dashboard created with new Dataviz.

https://www.npmjs.com/package/@keen.io/dataviz

woahdae commented 4 years ago

OK, yay for new things... but I JUST did a bunch of work getting on this major version! I'll check it out though, looks nice.

I was in this part of the code so I could figure out how to display historic data on one sparkline:

Hopefully the new Dataviz package has something similar? I had to really dig in to the code to figure out how to do this.

maciejrybaniec commented 4 years ago

@woahdae the metric chart with sparkline is on our roadmap - right now we support metric chart with custom icons.

woahdae commented 4 years ago

The biggest challenge is historic comparison, which currently one metric supports but only in absolute terms (we wanted percent change), and other charts throw an error if you try to chart different timeframes.

On Thu, May 14, 2020 at 10:38 PM Maciej Rybaniec notifications@github.com wrote:

@maciejrybaniec approved this pull request.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keen/keen-dataviz.js/pull/216#pullrequestreview-412359521, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGFQU7PMQS5VKB7Z34FOTRRTILVANCNFSM4M7I2WZA .

-- Sent from my phone

keen / keen-dataviz.js