Extract series picker into angular component #11905

diosmosis · 2017-07-30T22:58:22Z

This PR is based off #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 · 2017-09-24T19:40:48Z

plugins/CoreVisualizations/angularjs/series-picker/series-picker.component.html

+<div
+    class="jqplot-seriespicker"
+    ng-class="{open: $ctrl.isPopupVisible}"
+    ng-mouseenter="$ctrl.isPopupVisible = true"


@diosmosis just wondering... with components... can we as well use a syntax like controllerAs and refer to their controller name specifically to make sure we work in the right scope? Or do components not have this problem?

You can supply controllerAs, however the default for components is $ctrl: https://docs.angularjs.org/guide/component#comparison-between-directive-definition-and-component-definition

So you don't have to explicitly supply it. When coding this, I thought it was better not to have different controller variables all over the place (ie, instead of periodSelector in one component, seriesPicker in another, dataTable in another, everything uses $ctrl). Seems more consistent.

tsteur · 2017-09-24T19:41:24Z

plugins/CoreVisualizations/angularjs/series-picker/series-picker.component.html

+        class="jqplot-seriespicker-popover"
+        ng-if="$ctrl.isPopupVisible"
+    >
+        <p class="headline">{{ (this.multiselect ? 'General_MetricsToPlot' : 'General_MetricToPlot') | translate }}</p>


should this be $ctrl.multiselect?

tsteur · 2017-09-24T19:42:17Z

plugins/CoreVisualizations/angularjs/series-picker/series-picker.component.html

+            <label>{{ columnConfig.translation }}</label>
+        </p>
+        <p
+            ng-if="$ctrl.selectableRows && $ctrl.selectableRows.length"


BTW: I think it would simply do $ctrl.selectableRows.length as angular will check $ctrl.selectableRows automatically

tsteur · 2017-09-24T19:46:19Z

plugins/UserCountryMap/javascripts/vendor/kartograph.min.js

@@ -15,7 +15,9 @@
 *  You should have received a copy of the GNU Lesser General Public
 *  License along with this library. If not, see <http://www.gnu.org/licenses/>.
 *
+ * NOTE, manual modification made:
+ * - removed Function.bind implementation, does not work properly w/ angular 1.6


Is this file supposed to be changed?

Should have been changed in the upgrade to angular 1.6, not sure why it's changed here. I'll take a look.

Oh I didn't rebase the PR.

tsteur · 2017-09-24T19:48:35Z

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

diosmosis · 2017-09-28T01:19:16Z

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

tsteur · 2017-09-28T21:41:25Z

@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 · 2017-09-28T21:42:39Z

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 · 2017-09-28T21:47:24Z

plugins/CoreVisualizations/javascripts/seriesPicker.js

-                .attr('href', '#')
-                .html('+')
+            var seriesPicker = '<piwik-series-picker'
+                + ' multiselect="' + this.multiSelect + '"'


should we call some escaping here? I see it seems to hold a boolean value so should be fine just to prevent possible XSS it in the future (or maybe something like this.multiselect ? 'true' : false')? Also wondering: can we use old <div piwik-series-picker syntax with components? I checked at it seems to be still working with IE10 so reckon no other browsers will have trouble with this but not sure. I presume custom element names are supported everywhere else?

Good catch, completely missed it.

Also wondering: can we use old <div piwik-series-picker syntax with components?

I think angularjs 1.6 does not formally allow this (though oddly, angular 2+ supports it). See the 'restrict' part of https://docs.angularjs.org/guide/component#comparison-between-directive-definition-and-component-definition .

From some googling it might be allowed, but not intentionally.

I presume custom element names are supported everywhere else?

I can't find a proper browser support page, they all show support for the whole custom element feature, not just unknown tags. From what I remember only IE and some older versions of Firefox had this problem. Not sure what version of IE, but since angularjs 1.6 doesn't really allow attribute names for components, I'm assuming it works for whatever browser versions angular 1.6 supports.

tsteur · 2017-09-28T21:50:24Z

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 · 2017-09-28T23:42:09Z

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

…t positioning).

diosmosis · 2017-10-01T19:17:22Z

@tsteur Tweaked the positioning in treemap visualization so it would look better w/ this change: matomo-org/plugin-TreemapVisualization#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 · 2017-10-01T19:34:46Z

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:

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 👍

refs #13 and matomo-org/matomo#11905

tsteur · 2017-10-01T19:38:19Z

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 · 2017-10-01T20:22:50Z

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

diosmosis · 2017-10-02T02:25:46Z

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 · 2017-10-02T18:20:24Z

tests pass 👍

diosmosis added c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review labels Jul 30, 2017

mattab modified the milestone: 3.1.0 Aug 3, 2017

tsteur reviewed Sep 24, 2017

View reviewed changes

mattab modified the milestones: 3.3.0, 3.2.0 Sep 28, 2017

tsteur reviewed Sep 28, 2017

View reviewed changes

Extract series picker to new angular component (w/ less manual elemen…

e015f17

…t positioning).

diosmosis mentioned this pull request Oct 1, 2017

Make metric label full height of treemap header for more predictable positioning of series picker. matomo-org/plugin-TreemapVisualization#13

Merged

tsteur approved these changes Oct 1, 2017

View reviewed changes

tsteur added a commit to matomo-org/plugin-TreemapVisualization that referenced this pull request Oct 1, 2017

require Piwik 3.1.2-b2

f3d6a1d

refs #13 and matomo-org/matomo#11905

Update TreemapVisualization submodule to latest commit.

ce4f934

Update treemap screenshot.

fcc5bca

tsteur merged commit 359c3ec into matomo-org:3.x-dev Oct 2, 2017

diosmosis deleted the series-picker-component branch October 2, 2017 18:21

sgiehl mentioned this pull request Oct 24, 2017

Fixes seriespicker regression (changes to dashboard weren't saved anymore) #12223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract series picker into angular component #11905

Extract series picker into angular component #11905

diosmosis commented Jul 30, 2017 •

edited

tsteur Sep 24, 2017

diosmosis Sep 26, 2017

tsteur Sep 26, 2017

tsteur Sep 24, 2017

diosmosis Sep 26, 2017

tsteur Sep 24, 2017

tsteur Sep 24, 2017

diosmosis Sep 26, 2017

diosmosis Sep 26, 2017

tsteur commented Sep 24, 2017

diosmosis commented Sep 28, 2017

tsteur commented Sep 28, 2017

tsteur commented Sep 28, 2017

tsteur Sep 28, 2017

diosmosis Sep 28, 2017

tsteur commented Sep 28, 2017

diosmosis commented Sep 28, 2017

diosmosis commented Oct 1, 2017

tsteur commented Oct 1, 2017

tsteur commented Oct 1, 2017

diosmosis commented Oct 1, 2017

diosmosis commented Oct 2, 2017

tsteur commented Oct 2, 2017

Extract series picker into angular component #11905

Extract series picker into angular component #11905

Conversation

diosmosis commented Jul 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Sep 24, 2017

diosmosis commented Sep 28, 2017

tsteur commented Sep 28, 2017

tsteur commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Sep 28, 2017

diosmosis commented Sep 28, 2017

diosmosis commented Oct 1, 2017

tsteur commented Oct 1, 2017

tsteur commented Oct 1, 2017

diosmosis commented Oct 1, 2017

diosmosis commented Oct 2, 2017

tsteur commented Oct 2, 2017

diosmosis commented Jul 30, 2017 •

edited