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 dropdown and related directives #18214

Merged
merged 92 commits into from Nov 11, 2021
Merged

[Vue] migrate dropdown and related directives #18214

merged 92 commits into from Nov 11, 2021

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Oct 25, 2021

Description:

This PR is based off of #18213.

Changes:

  • add @types/materialize-css for materialize typings.
  • migrate focus-anywhere-but-here directive.
  • migrate focus-if directive.
  • migrate menudropdown directive.

image

Review

…se v-model, add event parameters to createAngularAdapter, allow translate to use variadic args or one string array + rebuild
…ndling to just reset back to the previous date since it was easier in vue to do that)
@diosmosis
Copy link
Member Author

Tests should be passing now.

@diosmosis
Copy link
Member Author

diosmosis commented Nov 9, 2021

@sgiehl this PR is ready for review (in case you weren't notified).

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. 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

public function aggregateMultipleReports()
{
$this->getProcessor()->processDependentArchive('VisitsSummary', VisitFrequencyAPI::NEW_VISITOR_SEGMENT);
$this->getProcessor()->processDependentArchive('VisitsSummary', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT);
Copy link
Member

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? 🤔

Copy link
Member Author

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;
Copy link
Member

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?

Copy link
Member Author

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>";
Copy link
Member

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.

Copy link
Member Author

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.

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

Added some docs related to the directive pattern: matomo-org/developer-documentation#582

@diosmosis diosmosis added the Needs Review PRs that need a code review label Nov 10, 2021
@diosmosis
Copy link
Member Author

@sgiehl updated

@sgiehl
Copy link
Member

sgiehl commented Nov 10, 2021

@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.

  • When an element is chosen, the drop down isn't closed any longer
  • When typing a keyword you currently need to press enter in order to get searched for it, before it was search automatically with each filled in letter

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

@sgiehl those issues should be fixed.

@diosmosis diosmosis added the Needs Review PRs that need a code review label Nov 10, 2021
@sgiehl
Copy link
Member

sgiehl commented Nov 11, 2021

@diosmosis awesome. everything seems to work as expected now 👍

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Nov 11, 2021
@sgiehl sgiehl merged commit 680ac30 into 4.x-dev Nov 11, 2021
@sgiehl sgiehl deleted the vue-dropdown branch November 11, 2021 10:53
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