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-period-selector angularjs component #18382

Merged
merged 317 commits into from Dec 3, 2021

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Nov 26, 2021

Description:

This PR is based off of #18352.

Changes:

  • Migrate piwik-period-selector directive.
  • Modify initialization of material selects in corehome.js to exclude ui datepicker controls.
  • Add new MatomoUrl.updateLocation() method that updates the hash or whole URL depending on whether we are in a widgetized context or not. Refactor comparisons store to use this method.
  • Modify ExpandOnClick/ExpandOnHover to allow string values for expander that reference Vue $refs. Without something like this, the directive will be initiated before the rest of the component is mounted, so refs for child elements will be undefined.

image

Review

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 comment. Otherwise the code is looking fine.
While clicking through the UI I found this regressions:

  • The compare dropdown has an incorrect z-index:
    image

  • In range selection: when changing a date in the input fields above the date chooser are the choosers only updated when the input field looses the focus. Before it was triggered with each change (guess on keypress or so)

matomo.js Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Dec 2, 2021
@diosmosis
Copy link
Member Author

@sgiehl

The compare dropdown has an incorrect z-index:

This happens on 4.5.0 too, it's a materialize issue.

In range selection: when changing a date in the input fields above the date chooser are the choosers only updated when the input field looses the focus. Before it was triggered with each change (guess on keypress or so)

Fixed + another related bug (will now only change the date if it is a full YYYY-MM-DD date).

Guess you accidentally committed the changes to the tracker js files.

Undid those changes.

@diosmosis diosmosis added the Needs Review PRs that need a code review label Dec 3, 2021
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 comment. Besides that the CustomAlerts submodule was changed in this PR as well. This should be reverted before merging. When that is changed and tests are passing, feel free to merge this one.

@diosmosis
Copy link
Member Author

There are a couple unrelated failures, one is fixed in another branch, but I'll create a new branch for it just so it's there. I'll look into the other one.

@diosmosis diosmosis merged commit 061f578 into 4.x-dev Dec 3, 2021
@diosmosis diosmosis deleted the vue-period-selector branch December 3, 2021 12:38
@tassoman
Copy link
Contributor

tassoman commented Dec 3, 2021

Hello there, does this migration Vue makes possible to override/modify the period selector calendar?
My Plugin RerUserDates rely on this but using angular, cannot hook in it .. 🤔

@diosmosis
Copy link
Member Author

@tassoman how specifically do you override/modify it? I can't see any angularjs code in the plugin.

@diosmosis
Copy link
Member Author

Hi @tassoman, I took another look and if you mean the HTML element removal your plugin does then it won't work with Vue anymore. Vue uses a virtual DOM and keeps HTML in sync with it, so if you remove an element, it will just be put back again. I would suggest a different approach:

  1. when you determine the element should not be there, add a css class to html (eg, remove-period-option)

  2. then in a LESS file hide the element based on that, eg:

    html.remove-period-option {
        #periodString ... {
            display: none;
        }
    }
    

@tassoman
Copy link
Contributor

tassoman commented Dec 6, 2021

Thanks for inspecting my code. I was stuck after the adoption of Angular.
I'm going to try this trick, looks like quite short and straight. Whithout involving diving deep in the ui frontend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants