[Vue] migrate piwik-reporting-menu directive to vue #18423

diosmosis · 2021-12-01T08:11:44Z

Description:

Changes:

Add jquery.js (no min.js) to asset ordering so it can be specified in addJavaScript files if its needed to debug jquery. Without this, it will be added out of order.
Migrate side-nav directive.
Migrate reporting-menu model service to store.
Migrate piwik-reporting-menu directive to vue.
Add a $timeout() call after AjaxHelper finishes a request (since angularjs triggers a digest after $http uses).
Move URL comma replacing logic to MatomoUrl.
Allow prepending notifications even if not going to a modal dialog.
Make reporting pages store adapter's more immutable.
Add clone helper method for creating angularjs adapters (angularjs adapter results are not allowed to modify Vue store data, but must still be mutable because angularjs expects it).
Fix to jqueryNativeEventTrigger.ts for click events on links.
Add window.widgetsHelper typings to index.d.ts.

Review

* use jQuery click for expand on click * undo submodule change

…field appearing in a select option list, but only at first

diosmosis · 2021-12-05T03:47:49Z

Tests should be passing for this one. Fixed a bunch of smaller bugs towards the end.

core/AssetManager/UIAssetFetcher/JScriptUIAssetFetcher.php

sgiehl

Left a couple of comments for stuff I found while looking through the code & test changes. Did not yet complete the manual testing. will add further comments if I find anything else...

sgiehl · 2021-12-07T15:12:06Z

plugins/CoreHome/vue/src/SideNav/SideNav.adapter.ts

+        oldValue: null,
+        modifiers: {},
+        dir: {},


Just out of curiosity: Are those attributes needed for anything? Can't see that they are used anywhere 🤔

These are required by the DirectiveBinding interface in Vue. If you remove them, phpstorm should complain. Typescript will also complain, but possibly not when calling vue:build. For the adapter we're passing a fake object just to trigger the directive logic correctly.

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.store.ts

plugins/Dashboard/tests/UI/DashboardManager_spec.js

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

plugins/CoreHome/vue/src/SiteSelector/SiteSelector.adapter.ts

sgiehl

Had a quick look at the UI file changes. Seems one css change needs to be reverted. And on the PrivacyManager tests it looks like that before the All Websites entry was preselected in the site selector. Now that switched to a specific site. Would it be easy to change that? Otherwise we could also define that as expected behavior and add according attributes to the site selector on the privacy settings pages, so that all websites is preselected

plugins/CoreHome/stylesheets/layout.less

diosmosis · 2021-12-08T09:46:00Z

@sgiehl

And on the PrivacyManager tests it looks like that before the All Websites entry was preselected in the site selector. Now that switched to a specific site. Would it be easy to change that?

Oh, I thought it didn't have an initial site set, but it is initialized in the controller. Looked into a bit, this is a different issue, though I'm not sure what... Setting data on the vue app isn't working, it doesn't propagate to the root component in the app. Will see if I can figure it out.

diosmosis · 2021-12-08T10:24:59Z

@sgiehl fixed it w/ an extra nextTick(). Not sure why this works, but it seems to.

plugins/CoreHome/stylesheets/layout.less

plugins/CoreHome/vue/src/AjaxHelper/AjaxHelper.ts

sgiehl · 2021-12-08T16:26:24Z

@diosmosis there still seem to be some changes in the UI files that look unexpected. The usermanagers results changed, but imho they shouldn't have

peterhashair

working as expected, I think the test is being fixed.

diosmosis · 2021-12-08T23:25:37Z

@sgiehl: @peterhashair was faster by a couple minutes, but it looks like the change was a fix to some broken behavior, the first failure is for role_for.png, in the test we select for a site named "relentless": https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/UsersManager/tests/UI/UsersManager_spec.js#L68, but in the 4.x-dev screenshot the wrong site is selected. Will merge in a couple hours if there are no further comments.

sgiehl · 2021-12-09T08:35:20Z

@diosmosis I guess some ui changes might still be some sort of incorrect:
tests/UI/expected-screenshots/Menus_admin_loaded.png shouldn't have been updated. Guess that one will now fail on 4.x-dev. Maybe you can directly fix that there.
plugins/Dashboard/tests/UI/expected-screenshots/DashboardManager_removed.png did you check why the menu switched from being displayed fully, to be collapsible? Fo me it looks like the menu isn't updated anymore when a dashboard has been removed.

diosmosis · 2021-12-09T08:59:49Z

Maybe you can directly fix that there.
plugins/Dashboard/tests/UI/expected-screenshots/DashboardManager_removed.png did you check why the menu switched from being displayed fully, to be collapsible?

This works for me locally (when using tests/PHPUnit/proxy). It might be a race condition. The menu is updated, that's something I specifically had to fix (specifically due to a race condition w/ the jquery dashboard code not waiting for dashboard load to finish) though it might be possible the fix is in a different PR. I'll look a bit more to see if I can reproduce it.

diosmosis added 30 commits November 17, 2021 14:47

fix a couple more bugs

da7f33d

use jQuery click for expand on click

bb28514

undo submodule change

c38b37f

rebuild vue

8348784

Merge branch 'vue-expand-on-click-bug' into vue-form-field

7fd2efd

Merge branch 'vue-siteselector' into vue-form-field

3d3f1f7

Merge branch '4.x-dev' into vue-form-field

6b2b737

get vue files to run

e8e9d91

missing element

2c0c35c

more fixes

f78a922

get more fields working properly

c28f7ba

rebuild

70390db

fix even more issues

64bebb1

[Vue] use jQuery click for expand on click (#18341)

6a3e90c

* use jQuery click for expand on click * undo submodule change

migrate piwik-field directive and fix some issues

97add93

forgot to add migrated directive files and get demo.twig to work

6920cdb

use event target

1010122

make sure angularjs and vue do not share data

6ca3d20

fix ng model mapping

5383bc0

fix a couple more issues

7408f5f

fix a couple more bugs

d025da8

more fixes

6890491

mimic existing behavior where an empty value will result in an extra …

d9c3531

…field appearing in a select option list, but only at first

another fix

a5824b0

built vue files

e492a96

Merge branch '4.x-dev' into vue-form-field

76cd6a3

more fixes

36dd6e7

do not use UMD if vue/src is not present

bb70c09

use non-minified angularjs if in development mode

ca40384

deprecate custom template file property

2efe403

diosmosis added 5 commits December 4, 2021 06:19

fix usersettings test failure

ee6de95

rebuild vue

e5b31c4

more siteselector tweaks.

c7a17d6

another siteselector issue

ad41d70

update screenshots

69b69db

diosmosis added 2 commits December 4, 2021 20:26

update screenshot and try to fix random failure

6345c8e

Merge branch '4.x-dev' into vue-reporting-menu

a62f78b

diosmosis commented Dec 6, 2021

View reviewed changes

core/AssetManager/UIAssetFetcher/JScriptUIAssetFetcher.php Show resolved Hide resolved

Merge branch '4.x-dev' into vue-reporting-menu

2c0f0e2

sgiehl requested changes Dec 7, 2021

View reviewed changes

apply review feedback

2062bfb

sgiehl requested changes Dec 8, 2021

View reviewed changes

plugins/CoreHome/stylesheets/layout.less Outdated Show resolved Hide resolved

fix ngmodel render issue w/ extra nextTick()

6fc6337

diosmosis force-pushed the vue-reporting-menu branch from db916f2 to 6fc6337 Compare December 8, 2021 10:28

sgiehl reviewed Dec 8, 2021

View reviewed changes

plugins/CoreHome/stylesheets/layout.less Outdated Show resolved Hide resolved

fix css

0739b31

sgiehl reviewed Dec 8, 2021

View reviewed changes

plugins/CoreHome/vue/src/AjaxHelper/AjaxHelper.ts Outdated Show resolved Hide resolved

diosmosis added 3 commits December 8, 2021 06:06

fix test

1747f0c

rebuild vue

a2ebca0

update screenshots

3556563

peterhashair approved these changes Dec 8, 2021

View reviewed changes

diosmosis merged commit c77bd82 into 4.x-dev Dec 9, 2021

diosmosis deleted the vue-reporting-menu branch December 9, 2021 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vue] migrate piwik-reporting-menu directive to vue #18423

[Vue] migrate piwik-reporting-menu directive to vue #18423

diosmosis commented Dec 1, 2021 •

edited

diosmosis commented Dec 5, 2021

sgiehl left a comment

sgiehl Dec 7, 2021

diosmosis Dec 8, 2021

sgiehl left a comment

diosmosis commented Dec 8, 2021

diosmosis commented Dec 8, 2021

sgiehl commented Dec 8, 2021

peterhashair left a comment

diosmosis commented Dec 8, 2021

sgiehl commented Dec 9, 2021

diosmosis commented Dec 9, 2021

[Vue] migrate piwik-reporting-menu directive to vue #18423

[Vue] migrate piwik-reporting-menu directive to vue #18423

Conversation

diosmosis commented Dec 1, 2021 • edited

Description:

Review

diosmosis commented Dec 5, 2021

sgiehl left a comment

Choose a reason for hiding this comment

sgiehl Dec 7, 2021

Choose a reason for hiding this comment

diosmosis Dec 8, 2021

Choose a reason for hiding this comment

sgiehl left a comment

Choose a reason for hiding this comment

diosmosis commented Dec 8, 2021

diosmosis commented Dec 8, 2021

sgiehl commented Dec 8, 2021

peterhashair left a comment

Choose a reason for hiding this comment

diosmosis commented Dec 8, 2021

sgiehl commented Dec 9, 2021

diosmosis commented Dec 9, 2021

diosmosis commented Dec 1, 2021 •

edited