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 comparisons service + component #18193
Conversation
…ty method (all untested)
…se v-model, add event parameters to createAngularAdapter, allow translate to use variadic args or one string array + rebuild
…s in between nodes while vue 3 does not
result[key].push(value); | ||
} else { | ||
result[key] = value; | ||
} |
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.
Modified this function to handle arrayParam[]=...
parameters. It will notice phpArrayParam[complexKey]=...
type parameters, but will ignore the key type. (These are used in heatmaps if I recall correctly, so thought I'd mention.)
@sgiehl (and @peterhashair if you're reviewing the Vue PRs as well), this should be ready for review. It got a lot bigger than I expected, and also includes new patterns for dealing w/ URLs/routing and $rootScope events. It would be good if it got a thorough review, but it is bigger than I expected it would be... |
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.
I had a look through all the changes. But to be honest I'm not very experienced with type script, so I'm not able to say much about it. maybe @peterhashair can say more on that.
Nevertheless I spent some time clicking through the comparison UI. So far I found some javascript errors popping up in the console when typing a custom compare period:
e6c3cb3
to
8765676
Compare
@sgiehl fixed the last UI tests and fixed the javascript errors you mentioned. |
@sgiehl FYI added related documentation for the store pattern and dealing with the URL here: matomo-org/developer-documentation#581 |
@diosmosis javascript errors are gone 👍 Guess this one also needs to be updated: https://builds-artifacts.matomo.org/matomo-org/matomo/vue-comparisons/50661/PeriodSelector_custom_comparison.png |
@tsteur do you know if it's ok to merge this or if I should wait a bit more for other reviews? |
@diosmosis I reckon it's good to merge 👍 |
Description:
This PR is based off of #18166.
Note: I checked the comparisons UI tests locally and they passed, but I checked nothing else. So this PR is blocked until the CI issues are dealt with.
Changes:
restrict: 'E'
in createAngularJsAdapter for use w/ migrating AngularJS components.Migrating $rootScope events
In angularjs, $rootScope.$emit is used as a way to implement pub/sub throughout the frontend. We will of course not be able to use this mechanism with Vue, so I started replacing it here.
Instead of $rootScope, we are just using
new CustomEvent
andwindow.dispatch
directly (so we don't need to depend on another library). This functionality is exposed in theMatomo
service, viaMatomo.on/Matomo.off/Matomo.postEvent
.During the migration,
$rootScope.$emit
is overridden to use Matomo.postEvent and Matomo.postEvent will use the old $rootScope.$emit. This way, $rootScope.$emit calls will trigger event handlers that still use $rootScope and handlers that use Matomo.on. As will Matomo.postEvent calls. Before Matomo 5 is released, this code will be removed.Similarly,
$broadcast
will also call Matomo.postEvent, so Matomo.on can be used w/ events like$locationChangeSuccess
.Migrating use of $location
AngularJS code that uses $location will be replaced with simply directly reading and writingwindow.location.hash
. This is the simplest approach and allows us to keep our custom hash based routing.The above approach will not work until AngularJS is removed, because $location events, like $locationChangeSuccess, are triggered before changes to the hash/URL are made. So now, if comparisons listens to the hashchange, it will execute it's event handler AFTER others like the reporting page handler, which breaks comparison since the comparison will not be updated at that point. Currently, the ordering works because the comparison event handler to $locationChangeSuccess is added before the reporting page one, simply because the JS file is loaded afterwards.
This PR replaces that system by creating a store for parsed URL information in the MatomoUrl class, then using Vue computed properties to process it. The computed properties are used in Vue components, so when the URL is updated, the specific components that depend on that data automatically notice the change and update. This means event handler order is entirely irrelevant, and there is only one part of the code that parses the URL and one part of the code that uses $locationChangeSuccess. (This is, as I understand it, the accepted way to implement routing in modern state driven UIs (at least in React), where the URL is treated as a piece of application state with derived/computed state at various parts of the application).
For comparisons, this looks like this:
ref()
properties.ref()
values to compute a query parameter object (ie,MatomoUrl.parsed.value.period
).MatomoUrl.parsed
property to parse segment/period comparison information & other information.ref()
properties mentioned above, and every computed property will automatically be aware of the change, and update.When angularjs is removed entirely, we can stop listening to $locationChangeSuccess in MatomoUrl, and use hashchange.
Existing angularjs code can still use $location.
Review