@diosmosis opened this Pull Request on October 25th 2021 Member

Description:

This PR is based off of https://github.com/matomo-org/matomo/pull/18213.

Changes:

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

image

Review

@peterhashair commented on November 2nd 2021 Contributor

I read through all the file changes, I think maybe we can push plugins/vue/dist/** to .gitignore, and build them through Github actions. But I guess the problem is when someone pulls the branch it breaks the page.

@diosmosis commented on November 2nd 2021 Member

I read through all the file changes, I think maybe we can push plugins/vue/dist/** to .gitignore,

Plugins are required to be distributed with those files since users are not expected to have node installed. At the moment this means keeping the files in the repo. If at some point we make it required to have node and to build the JS for git deployments, then we could only build in matomo-package + in the marketplace when publishing a plugin, then the built JS doesn't necessarily need to be in the repo. But that's a lot of work I'm guessing not a high priority right now.

@diosmosis commented on November 2nd 2021 Member

@peterhashair was there a specific problem you wanted to solve w/ ignoring the compiled JS?

@peterhashair commented on November 2nd 2021 Contributor

@diosmosis just try to pull this PR but got an error somehow on the dashboard. It could be I merge those conflicts. See the screenshot or missing a build step.
Screen Shot 2021-11-03 at 11 53 53 AM
w

@diosmosis commented on November 3rd 2021 Member

This PR is based off of https://github.com/matomo-org/matomo/pull/18213 which has more changes. I haven't updated this PR yet. Does this error happen on that PR as well?

@peterhashair commented on November 3rd 2021 Contributor

@diosmosis yes, some error. #18213

@diosmosis commented on November 3rd 2021 Member

@peterhashair I have no such issue on the vue-comparisons branch (the displayed error has to do w/ evolution icons and is unrelated):

image

Can you please make sure the new files are being used and provide more information, such as the full error message in the console, the URL, the list of widgets in the dashboard, etc.

EDIT: Actually from the error message it looks like the JS for CoreHome.umd.js is wrong somehow or not up to date. Can you run vue:build and see if it fixes the issue?

@peterhashair commented on November 8th 2021 Contributor

@diosmosis beside this PR. I got 3 questions.

  • Once we fully merged to VUE, I guess we don't need *.adpater.ts. we can just call <DateRangePicker/> sth like that inside of *.twig?
  • why don't we use a VUE library (eg: Vuetify, Buffy, Elements, etc) for date-picker, etc.
  • Are we going to make a summary page for all the VUE components we made and a guide for how to use them?
@diosmosis commented on November 8th 2021 Member

@peterhashair

Once we fully merged to VUE, I guess we don't need .adpater.ts. we can just call sth like that inside of .twig?

No, vue templates can't go directly into twig, Vue doesn't scan HTML like angularjs does. The twig templates will likely have to converted to Vue components and initiated via a Matomo-specific mechanism in twig templates.

why don't we use a VUE library (eg: Vuetify, Buffy, Elements, etc) for date-picker, etc.

I'm only converting angularjs/twig to Vue, nothing else. Introducing other libraries to replace jquery UI & materialize is extra work and would need to be accepted & prioritized by @tsteur / @mattab.

Are we going to make a summary page for all the VUE components we made and a guide for how to use them?

That is not currently part of the plan, but I can see it as possible. Again, @tsteur / @mattab would have to accept and prioritize it. Like introducing or replacing other libraries, it would not be done by me.

@peterhashair commented on November 8th 2021 Contributor

@diosmosis right, thanks for the explanation, that makes more sense. 😄

@tsteur commented on November 8th 2021 Member

@peterhashair this is all out of scope for now indeed (I mentioned last week). For now it's purely migrating each component step by step from angular to Vue. Once this is all done we can look at other UI libs when there is a UI redesign or when something else comes up.

Re the documentation: We have this where the most common ones are documented and shown: https://matomo-url//index.php?module=Morpheus&action=demo&idSite=1&period=day&date=yesterday.

Eventually we can identify the ones that are used more often and add them there but it's not too much of a priority since many of them are self explaining. It'll be good to have though at some point so it's clear what UI components can be re-used as it's otherwise hard to know which ones exist. Maybe create a new issue for it?

@diosmosis commented on November 8th 2021 Member

Tests should be passing now.

@diosmosis commented on November 9th 2021 Member

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

@diosmosis commented on November 9th 2021 Member

Added some docs related to the directive pattern: https://github.com/matomo-org/developer-documentation/pull/582

@diosmosis commented on November 10th 2021 Member

@sgiehl updated

@sgiehl commented on November 10th 2021 Member

@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
@diosmosis commented on November 10th 2021 Member

@sgiehl those issues should be fixed.

@sgiehl commented on November 11th 2021 Member

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

This Pull Request was closed on November 11th 2021
Powered by GitHub Issue Mirror