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 dropdown and related directives #18214
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
…ndling to just reset back to the previous date since it was easier in vue to do that)
…te if no translations are loaded
…larjs comment no longer being there)
Tests should be passing now. |
@sgiehl this PR is ready for review (in case you weren't notified). |
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. In addition I found this while testing:
- When selecting an item in the drop down (e.g. dashboards or goals selection) on a reporting page, the item is loaded, but the drop down menu isn't closed as before
plugins/VisitFrequency/Archiver.php
Outdated
public function aggregateMultipleReports() | ||
{ | ||
$this->getProcessor()->processDependentArchive('VisitsSummary', VisitFrequencyAPI::NEW_VISITOR_SEGMENT); | ||
$this->getProcessor()->processDependentArchive('VisitsSummary', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT); |
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.
Did you remove that one by accident? 🤔
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 guess I did... not sure how
@@ -22,54 +22,8 @@ | |||
restrict: 'A', | |||
link: function(scope, element, attr, ctrl) { | |||
|
|||
var isMouseDown = false; |
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.
Any reason why this directive wasn't fully removed like the others?
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.
No, I think I just forgot to review this one myself before pinging, apologies.
$str .= "<script type='text/javascript' src='plugins/CoreHome/angularjs/menudropdown/menudropdown.directive.js'></script>"; | ||
|
||
$coreHomeUmd = Development::isEnabled() ? 'CoreHome.umd.js' : 'CoreHome.umd.min.js'; | ||
$str .= "<script type='text/javascript' src='plugins/CoreHome/vue/dist/$coreHomeUmd'></script>"; |
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 guess this might be not needed anymore. If I have seen that correctly, the CoreUpdater and Installation Controller are both now adding the vue js files themselves, so they should be available already.
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.
They're adding the core Vue files (ie, vue.js + matomopolyfills), but not the CoreHome UMD. That still needs to be added... but maybe it's weird to have it here in LanguagesManager? Otherwise some future widget might have to do the same thing and it gets added twice. I'll just load it in CoreUpdater/Installation.
Added some docs related to the directive pattern: matomo-org/developer-documentation#582 |
@sgiehl updated |
@diosmosis the changes are looking fine. The only thing left is, that the drop down menu e.g. for goals or dashboards doesn't behave the same.
|
@sgiehl those issues should be fixed. |
@diosmosis awesome. everything seems to work as expected now 👍 |
Description:
This PR is based off of #18213.
Changes:
Review