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
[Vue] migrate piwik-field and related directives #18352
Conversation
…and parts of UI.Notification to Vue
…he same for other dependent data in the comparison store to allow vues to subscribe to the properties for changes to global state
@diosmosis I'm currently doing a review. Might take a while as the changes are quite huge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments while looking through the code only. Will now check tests & do some local testing.
Btw. We won't be able to merge this one, until #18406 is merged, as your branch already contains some of the commits from next_release
branch.
plugins/CoreHome/CoreHome.php
Outdated
@@ -164,7 +165,7 @@ public function getJsFiles(&$jsFiles) | |||
$jsFiles[] = "node_modules/jquery.scrollto/jquery.scrollTo.min.js"; | |||
$jsFiles[] = "node_modules/sprintf-js/dist/sprintf.min.js"; | |||
$jsFiles[] = "node_modules/mousetrap/mousetrap.min.js"; | |||
$jsFiles[] = 'node_modules/angular/angular.min.js'; | |||
$jsFiles[] = Development::isEnabled() ? 'node_modules/angular/angular.js' : 'node_modules/angular/angular.min.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory that might cause problems if someone enables development mode in a Matomo not installed from git. As the the unminified files is removed in builds.
plugins/CoreHome/angularjs/report-export/reportexport.popover.html
Outdated
Show resolved
Hide resolved
@diosmosis While clicking through the UI / tests I so far spotted this issues:
|
I'm done for now. Couldn't find anything else while clicking around. Feel free to re-request a review once everything is fixed. |
b9ce915
to
303fcf4
Compare
@sgiehl this should be ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diosmosis I have clicked through the issues I had reported and everything seems to work fine now. One last thing needs to be updated. It seems you have updated the CustomAlerts submodule to a specific branch, this might cause problems when we merge that. If there are changes required in the submodule please create PRs there. Guess we need to reset the submodule to 4.x-dev
here before merging. Otherwise good to merge, if tests are passing again.
@sgiehl I'll create a PR on CustomAlerts 👍 |
fixed a failing test. Will merge this now. @diosmosis feel free to ping me, once the PR for custom alerts is created. |
Description:
This PR is based off of #18292.
piwik-field
is a recursive directive so they all had to be converted at once, so this PR is also quite large.Changes:
piwik-field
directive to the vue Field directive. Note: thetemplateFile
property is not longer supported. If plugin developers use it in the wild, we'll have to provide a different way of extending Field.matomo-multi-pair-field
directive to MultiPairField.matomo-field-array
directive to FieldArray.piwik-form-field
directive to FormField. Every template that was included in the angularjs directive was converted to a new vue component used by FormField.useExternalPluginComponent()
function.Review