@diosmosis opened this Pull Request on July 30th 2017 Member

This PR is based off https://github.com/piwik/piwik/pull/11857.

Ready for review, but won't resolve conflicts until #11857 is merged.

Changes:

  • Moved the SeriesPicker class logic to a new piwik-series-picker component. The SeriesPicker class stays for BC (used at least by the treemap visualization plugin).
  • Added ability to create an isolate scope to piwikHelper.compileAngularComponents.
  • (side effect) Fixed some issues w/ UI tests that include the series picker.
@tsteur commented on September 24th 2017 Owner

Left a few comments. Worked for me in general, also in IE10.

@diosmosis commented on September 28th 2017 Member

Made some changes, if the UI tests pass, should be good to go.

@tsteur commented on September 28th 2017 Owner

@diosmosis did you squash the last change or so? be good to keep the individual commits so we can more easily review what changed since last time. We can always squash when we merge.

@tsteur commented on September 28th 2017 Owner

FYI: this ui test is now failing maybe because the title is next to series picker? http://builds-artifacts.piwik.org/piwik/piwik/3.x-dev/24550/UIIntegrationTest_exampleui_treemap.png

@tsteur commented on September 28th 2017 Owner

Left a comment but looks good to merge otherwise. Only minor tweak needed maybe to prevent possible XSS in the future (currently wouldn't be possible). UI test might need a fix as well.

@diosmosis commented on September 28th 2017 Member

@tsteur didn't squash, rebased, there was only one different commit in this PR.

@diosmosis commented on October 1st 2017 Member

@tsteur Tweaked the positioning in treemap visualization so it would look better w/ this change: https://github.com/piwik/plugin-TreemapVisualization/pull/13

Think you could look at that today? Since the branch is in a fork I can't use it as the submodule commit in this PR w/o changing the submodule repo.

@tsteur commented on October 1st 2017 Owner

I tried to test it but couldn't as the treemap is not rendering for me (I highly doubt it is related to your PR... the canvas element etc has always 0 height) and looks like this:
image

The tests don't run as it seems to not be able to check out your treemap branch but I trust it is fixed and can be merged 👍

@tsteur commented on October 1st 2017 Owner

I merged the other branch in treemap plugin. can you maybe update the submodule to use the master branch and then tests will work?

@diosmosis commented on October 1st 2017 Member

Working on getting the tests to pass, will post a comment when it's all working.

@diosmosis commented on October 2nd 2017 Member

Updated the screenshot, if it passes, this PR is good to go. (Let me know if there's anything else to do for TreemapVisualization, release-wise).

@tsteur commented on October 2nd 2017 Owner

tests pass 👍

This Pull Request was closed on October 2nd 2017
Powered by GitHub Issue Mirror