Skip to content
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

Merged
merged 291 commits into from Dec 1, 2021
Merged

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Nov 19, 2021

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:

  • Migrate piwik-field directive to the vue Field directive. Note: the templateFile property is not longer supported. If plugin developers use it in the wild, we'll have to provide a different way of extending Field.
  • Migrate matomo-multi-pair-field directive to MultiPairField.
  • Migrate matomo-field-array directive to FieldArray.
  • Add extra parameters to createAngularJsAdapter() scope property transform function.
  • Migrate 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.
  • Add new mechanism to support extending form fields that allows passing in a Vue component, rather than angularjs template (this method is still supported for BC, until Matomo 5).
  • Do not use Vue UMDs if there is a dist folder w/ files but no src folder (can happen during development).
  • Fix bug visible in 4.x-dev w/ report export: changing the report limit selector did not change the filter_limit in the export URL. For some reason, angularjs couldn't modify $parent.reportLimit, possibly because there is also a scope.reportLimit.
  • Put the defineAsyncComponent() function used to get components from other plugins when we don't know if they're loaded into the new useExternalPluginComponent() function.
  • Removed the selectedSite internal state in the SiteSelector component, it now follows the v-model contract properly.

image

image

Review

…he same for other dependent data in the comparison store to allow vues to subscribe to the properties for changes to global state
@sgiehl
Copy link
Member

sgiehl commented Nov 30, 2021

@diosmosis I'm currently doing a review. Might take a while as the changes are quite huge.
Looking through some of the expected UI file changes I have already seen, that the title for text FieldArrays are not displayed for each input anymore. See d343d20?short_path=ecaef42#diff-ecaef42b8254004961e2206553ed6b910ac38155a6fa329264534e400d068fc0
Feel free to fix that already, while I'm still reviewing the code.

Copy link
Member

@sgiehl sgiehl left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -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';
Copy link
Member

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/CorePluginsAdmin/vue/src/FormField/FormField.vue Outdated Show resolved Hide resolved
plugins/CorePluginsAdmin/vue/src/FormField/FormField.vue Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member

sgiehl commented Nov 30, 2021

@diosmosis While clicking through the UI / tests I so far spotted this issues:

  • Opening the configuration for GoogleAnalyticsImporter (which is configured) results in a JavaScript error:
    image
    The same also happens for CustomTranslations plugin, same error caused by multitupletext field
  • The expected test file for mobile messaging changed here: f0d1f27?short_path=0ad0702#diff-0ad0702aa329c2b750bd3f088bc8316ac7d524d1b98f06a2e01d930dfd4d53ed
    I don't think we should update the test file, but adjust the test instead to work the same again. The problem seems to be, that you kind of fixed an issue in select boxes. Before the mobile provider select box had an empty entry in the beginning. After the vue changes that empty entry is gone (which imho is correct). So guess we need to adjust this:
    $('[name=smsProviders] ul li:nth-child(3)').click();
  • Opening the AB-Tests management, results in:
    image
    Same happens for FormAnalytics and when editing a goal
  • Opening the tracking code generator doesn't show the tracking codes anymore till some property is changed. I think this regressed earlier, as it worked in 4.5.0, but doesn't on 4.x-dev. (That's also the reason for the failing tests) If that's unrelated to Vue, feel free to move that to a new issue.
  • On the GDRP Tools page, there is another error:
    image

@sgiehl
Copy link
Member

sgiehl commented Nov 30, 2021

I'm done for now. Couldn't find anything else while clicking around. Feel free to re-request a review once everything is fixed.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Nov 30, 2021
@diosmosis
Copy link
Member Author

@sgiehl this should be ready for review again

Copy link
Member

@sgiehl sgiehl left a 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.

@diosmosis
Copy link
Member Author

@sgiehl I'll create a PR on CustomAlerts 👍

@sgiehl
Copy link
Member

sgiehl commented Dec 1, 2021

fixed a failing test. Will merge this now. @diosmosis feel free to ping me, once the PR for custom alerts is created.

@sgiehl sgiehl merged commit 43c21c9 into 4.x-dev Dec 1, 2021
@sgiehl sgiehl deleted the vue-form-field branch December 1, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants