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 sparkline library with modern equivalent #12066

Merged
merged 14 commits into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@Findus23
Member

Findus23 commented Sep 16, 2017

Fixes #12065
Fixes #12271
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

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Sep 16, 2017

Member

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

Member

Findus23 commented Sep 16, 2017

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Sep 16, 2017

Member

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

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

Member

Findus23 commented Sep 16, 2017

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

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23
Member

Findus23 commented Sep 16, 2017

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Sep 17, 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

Member

Findus23 commented Sep 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 18, 2017

Member

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?

Member

mattab commented Sep 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Sep 20, 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.

Member

sgiehl commented Sep 20, 2017

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 Findus23 referenced this pull request Sep 20, 2017

Closed

Allow adding featurepoints #2

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Sep 20, 2017

Member

Hi, I opened an feature request in the library, asking for an easy way to add dots:
davaxi/Sparkline#2

Member

Findus23 commented Sep 20, 2017

Hi, I opened an feature request in the library, asking for an easy way to add dots:
davaxi/Sparkline#2

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 11, 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 (davaxi/Sparkline#3) gets merged, so we can use the upstream version.

Member

Findus23 commented Nov 11, 2017

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 (davaxi/Sparkline#3) gets merged, so we can use the upstream version.

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Nov 12, 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

Member

sgiehl commented Nov 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 12, 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.

Member

Findus23 commented Nov 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 12, 2017

Member

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 👍

Member

mattab commented Nov 12, 2017

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 👍

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 12, 2017

Member

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

Invalid hexadecimal value

Stack trace




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

mattab commented Nov 12, 2017

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

Invalid hexadecimal value

Stack trace




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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 13, 2017

Member

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)

Member

mattab commented Nov 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 13, 2017

Member

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

Member

mattab commented Nov 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 13, 2017

Member

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

Member

Findus23 commented Nov 13, 2017

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 13, 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

Member

Findus23 commented Nov 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Nov 13, 2017

Member

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

Member

sgiehl commented Nov 13, 2017

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 18, 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.

Member

Findus23 commented Nov 18, 2017

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

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Nov 19, 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 #162C4A
Afterwards we can update the screenshots to be able to merge afterwards

Member

sgiehl commented Nov 19, 2017

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 #162C4A
Afterwards we can update the screenshots to be able to merge afterwards

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 19, 2017

Member

Fixed and update the UI tests

Member

Findus23 commented Nov 19, 2017

Fixed and update the UI tests

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 19, 2017

Member
  • 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"}
Member

mattab commented Nov 19, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 19, 2017

Member
  • 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
Member

mattab commented Nov 19, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 19, 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)
Member

Findus23 commented Nov 19, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 19, 2017

Member

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

Member

mattab commented Nov 19, 2017

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Nov 23, 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 43ee41b

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

Member

Findus23 commented Nov 23, 2017

@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 43ee41b

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

@Findus23

This comment has been minimized.

Show comment
Hide comment
@Findus23

Findus23 Feb 10, 2018

Member

@mattab

Is there anything that needs to be changed (apart from the UI tests) before this can be merged.

BTW: I noticed that this library was already included in Piwik 0.1.1, so it seems like I'm removing one of the oldest parts of Piwik 🙂

Member

Findus23 commented Feb 10, 2018

@mattab

Is there anything that needs to be changed (apart from the UI tests) before this can be merged.

BTW: I noticed that this library was already included in Piwik 0.1.1, so it seems like I'm removing one of the oldest parts of Piwik 🙂

@Findus23 Findus23 requested a review from mattab Feb 10, 2018

@sgiehl

Left some comments. Besides that it looks good to me.
As the new library brings a lot of unneeded files with it (like tests and even the autoload.php, which isn't used with composer), we need to update our build script to remove them. Maybe you could already prepare a PR for that.

@Findus23 Findus23 removed the request for review from mattab Feb 10, 2018

@mattab mattab modified the milestones: 3.5.0, 3.4.0 Feb 16, 2018

@sgiehl

sgiehl approved these changes Jun 14, 2018

Rebased the branch and added a little improvements. The last UI results are looking good, so guess we could update them and merge...

@sgiehl sgiehl merged commit cfe8dfd into 3.x-dev Jun 18, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@sgiehl sgiehl deleted the sparklines branch Jun 18, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

replace sparkline library with modern equivalent (matomo-org#12066)
* replace sparkline library with modern equivalent

* test setting $enableSparklineImages to true for UI tests

* update LEGALNOTICE

* add minimum and maximum dots

* further improve sparklines

- no minimum/maximum if they are the same
- add offset to the top so that maximum isn't stuck at the border
- doubled resolution of image as it was a bit blurry and it's still
<1.5KB

* fix display size of sparklines

* make Sparklines red

* fix sparkline size in rowEvoluton

* change line colors to black

* lastPoint and padding

* update Sparkline to new version

* remove possibility to disable sparklines ($enableSparklineImages)

* handle formatted numbers

* Updates UI files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment