@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

@Findus23 commented on November 11th 2017 Member

grafik

Now with minimum and maximum dots. I left out the end dot as I hope everyone is able to quickly see the end of the graph without it.

Hopefully my pull request (https://github.com/davaxi/Sparkline/pull/3) gets merged, so we can use the upstream version.

@sgiehl commented on November 12th 2017 Member

awesome! One thing that came to my mind when looking at the UI test results: Does it make sense to show the maximum and minimum dot when they both have the same value. If a sparkline always has 0 value (or any other constant value) it imho doesn't make that much sense to show the dots

@Findus23 commented on November 12th 2017 Member

Now there are no min/max dots if they are the same.

In addition I added a padding to the top of the graph as otherwise the maximum would be always the top of the image and the dot was cut of

And I increased the image resolution, maybe it's looking better this way.

We could change the style a bit to better match the Piwik style, but I'm the wrong person to ask.

@mattab commented on November 12th 2017 Owner

Nice @Findus23 that you added the feature upstream!

And I increased the image resolution, maybe it's looking better this way.

Does it mean the sparkline will be "retina" compatible and look great on high res screen? that would be a nice benefit :+1:

@mattab commented on November 12th 2017 Owner

I tried the PR and after running composer install, when loading a sparkline image, i get:

Invalid hexadecimal value

Stack trace

<a href='/0'>#0</a> vendor/davaxi/sparkline/src/Sparkline.php(271): Davaxi\Sparkline->colorHexToRGB(NULL)
<a href='/1'>#1</a> core/Visualization/Sparkline.php(143): Davaxi\Sparkline->setFillColorHex(NULL)
<a href='/2'>#2</a> core/Visualization/Sparkline.php(56): Piwik\Visualization\Sparkline->setSparklineColors(Object(Davaxi\Sparkline))
<a href='/3'>#3</a> plugins/CoreVisualizations/Visualizations/Sparkline.php(59): Piwik\Visualization\Sparkline->main()
<a href='/4'>#4</a> core/Plugin/Report.php(317): Piwik\Plugins\CoreVisualizations\Visualizations\Sparkline->render()
<a href='/5'>#5</a> plugins/CoreHome/Controller.php(58): Piwik\Plugin\Report->render()
<a href='/6'>#6</a> [internal function]: Piwik\Plugins\CoreHome\Controller->renderReportWidget(Object(Piwik\Plugins\API\Reports\Get))
<a href='/7'>#7</a> core/FrontController.php(544): call_user_func_array(Array, Array)
<a href='/8'>#8</a> core/FrontController.php(137): Piwik\FrontController->doDispatch('API', 'get', Array)
<a href='/9'>#9</a> core/dispatch.php(34): Piwik\FrontController->dispatch()
<a href='/10'>#10</a> index.php(27): require_once('/home/matt/dev/...')
<a href='/11'>#11</a> {main}
@mattab commented on November 13th 2017 Owner

Also got a Notification showing WARNING: core/Visualization/Sparkline.php(143): Notice - Undefined index: fillColor - Piwik 3.2.0 - Please report this message in the Piwik forums: https://forum.piwik.org (please do a search first as it might have been reported already)

@mattab commented on November 13th 2017 Owner

To keep things simple, we should try to make sparklines look exactly the same (sizes, colors, thickness of lines, size of dots, etc.). Currently they look quite different:

not same

@Findus23 commented on November 13th 2017 Member

Seems like this pull request already fixes #12271, as the new library displays the following on empty data:
grafik

@Findus23 commented on November 13th 2017 Member

I am not sure why the new color (fillcolor) won't get submitted for you.
I kept the (interesting 😄) way of defining the colors via CSS and just added a new element.
https://github.com/piwik/piwik/pull/12066/files#diff-cfb90e629f5b699cd4245bb8c5267632R10
Have you regenerated the CSS and Javascript files?

The difference in style in your screenshot is because of the doubling of the resolution as the size doesn't seem to be fixed there. I'll check that

@sgiehl commented on November 13th 2017 Member

@Findus23 I guess @mattab meant they should look exactly like before:
sparline

@Findus23 commented on November 18th 2017 Member

As my pull request got merged and a new version got released, I changed the composer.json to point to the original library.

@sgiehl commented on November 19th 2017 Member

Would be awesome to merge that for one of the coming versions.
Would you adjust the sparklines so they look the same as before? So line color back to <a href='/162'>#162</a>C4A
Afterwards we can update the screenshots to be able to merge afterwards

@Findus23 commented on November 19th 2017 Member

Fixed and update the UI tests

@mattab commented on November 19th 2017 Owner
  • Haven't yet tested in details but looking at the UI screenshots, the dots are cut off. Could we add some padding inside the image to prevent any dots being cut off?
  • are the colors of the sparklines assigned from the URL parameters? as they could be themed and the colors are passed to the sparkline URL eg. &colors={"backgroundColor"%3A"%23ffffff"%2C"lineColor"%3A"%23162c4a"%2C"minPointColor"%3A"%23ff7f7f"%2C"maxPointColor"%3A"%2375bf7c"%2C"lastPointColor"%3A"%2355aaff"}
@mattab commented on November 19th 2017 Owner
  • also ideally we need a blue dot (lastPointColor) to show where the last value stands (ie. current selected period) so it's visually clear where the last value stands VS min/max
@Findus23 commented on November 19th 2017 Member
  • padding
    Last time I tried, I failed to add a padding, so I only added it vertically. I’ll need to take a deeper look into the library to fix that.
  • styling
    The graphs are coloured via the parameters which are created via the css styling. So any theme can change them by modifying the css (as before)
  • lastpoint
    I wasn’t sure if it was necessary so I haven’t added them yet. But that shouldn’t be that hard (apart from the padding issue)
@mattab commented on November 19th 2017 Owner

Thanks for the update!
lastPoint and adding some padding are I reckon needed for merge the PR and not lose features/styles :+1:

@Findus23 commented on November 23rd 2017 Member

@mattab, I think now I have all features.
I want to wait until my changes are merged upstream, but otherwise I think this is good to merge.

Personaly I'd prefer a more minimalistic style, so I think I'll publish a minimal theme looking similar to https://github.com/piwik/piwik/pull/12066/commits/43ee41be0cae9049cfffb70f23ec0ab8457e8a2a

That would also be a nice demo on how to style dynamic elements in Piwik.

@sgiehl commented on November 24th 2017 Member
@Findus23 commented on November 24th 2017 Member

@sgiehl
I am not sure what went wrong, but I fixed it now

Powered by GitHub Issue Mirror