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

Remove a bit of unused code in multichart logic #216

Merged
merged 1 commit into from
May 18, 2020

Conversation

woahdae
Copy link
Contributor

@woahdae woahdae commented May 13, 2020

Commit 89534f7 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?

Commit 89534f7 removed some code without removing some dependent
lines, which are now cruft. This removes those lines for clarity.
@maciejrybaniec
Copy link
Contributor

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

woahdae commented May 13, 2020

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:

Screen Shot 2020-05-13 at 4 31 14 PM

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

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

@woahdae
Copy link
Contributor Author

woahdae commented May 15, 2020 via email

@maciejrybaniec maciejrybaniec merged commit a3fc3b8 into keen:master May 18, 2020
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.

3 participants