@Findus23 opened this Pull Request on September 16th 2017 Member

Fixes #12065
Refs #8410

The original sparkline library wasn't updated for 12 years. The original author has written a new version, but it is intended to be used as a standalone file: https://github.com/jamiebicknell/Sparkline

But there is another modern php library which creates similar sparklines:
https://github.com/davaxi/Sparkline

I also tried to improve the color scheme but I am open for improvements.

Unfortunately it doesn't support adding dots, so there are no minimum/maximum points anymore (But I am not sure how useful they are)

This definitely needs some testing before merging, as I don't know all variants of sparklines in piwik.

@Findus23 commented on September 16th 2017 Member

Oddly the only failing tests have nothing to do with this change.
are there no ui tests of the all websites overview?

@Findus23 commented on September 16th 2017 Member

It seems like the sparklines aren't visible on the UI tests.

(tests/UI/expected-screenshots/MultiSitesTest_all_websites.png)

@Findus23 commented on September 16th 2017 Member
@Findus23 commented on September 17th 2017 Member

And I guess there was a reason for \Piwik\Visualization\Sparkline::$enableSparklineImages = false;
as the test ran endlessly:
https://travis-ci.org/piwik/piwik/builds/276259820

@mattab commented on September 18th 2017 Owner

Hi @Findus23

Feedback

  • the reason the sparklines are disabled in UI tests is because GD library generating the images used to generate different images (looking the same, but different bytes), depending on the GD library. And it was difficult to force developers to run the same version of GD locally as on travis CI. For sure It would be better if we could test sparklines in UI tests...

  • the min/max dots are in my opinion quite useful to have as it helps scan the sparklines quickly and provides insights

  • maybe we could just rename the Object class to another name to fix the issue, and keep our current sparklines with the min/max dots?
@sgiehl commented on September 20th 2017 Member

Those min/max dots should not be the only reason not to search for a new library. We should avoid to continue using such an old and not maintained library.

I would agree with renaming the Object class to have a quick fix for PHP 7.2. But we should still consider replacing the old lib.

@Findus23 commented on September 20th 2017 Member

Hi, I opened an feature request in the library, asking for an easy way to add dots:
https://github.com/davaxi/Sparkline/issues/2

Powered by GitHub Issue Mirror