@diosmosis opened this Pull Request on November 19th 2021 Member

Description:

This PR is based off of https://github.com/matomo-org/matomo/pull/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

@sgiehl commented on November 30th 2021 Member

@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 https://github.com/matomo-org/matomo/commit/d343d20a429bc86b205fb9762c498df84a566896?short_path=ecaef42#diff-ecaef42b8254004961e2206553ed6b910ac38155a6fa329264534e400d068fc0
Feel free to fix that already, while I'm still reviewing the code.

@sgiehl commented on November 30th 2021 Member

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

@sgiehl commented on November 30th 2021 Member

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

@diosmosis commented on December 1st 2021 Member

@sgiehl this should be ready for review again

@diosmosis commented on December 1st 2021 Member

@sgiehl I'll create a PR on CustomAlerts :+1:

@sgiehl commented on December 1st 2021 Member

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

This Pull Request was closed on December 1st 2021
Powered by GitHub Issue Mirror