@diosmosis opened this Pull Request on December 1st 2021 Member

Description:

Changes:

  • Add jquery.js (no min.js) to asset ordering so it can be specified in addJavaScript files if its needed to debug jquery. Without this, it will be added out of order.
  • Migrate side-nav directive.
  • Migrate reporting-menu model service to store.
  • Migrate piwik-reporting-menu directive to vue.
  • Add a $timeout() call after AjaxHelper finishes a request (since angularjs triggers a digest after $http uses).
  • Move URL comma replacing logic to MatomoUrl.
  • Allow prepending notifications even if not going to a modal dialog.
  • Make reporting pages store adapter's more immutable.
  • Add clone helper method for creating angularjs adapters (angularjs adapter results are not allowed to modify Vue store data, but must still be mutable because angularjs expects it).
  • Fix to jqueryNativeEventTrigger.ts for click events on links.
  • Add window.widgetsHelper typings to index.d.ts.

image

Review

@peterhashair commented on December 3rd 2021 Contributor

not sure if this is done in another PR, but it seems like the menu has a function that groups large datasets to a autocomplete search box

Expected
image

Current
image

@diosmosis commented on December 5th 2021 Member

Tests should be passing for this one. Fixed a bunch of smaller bugs towards the end.

@diosmosis commented on December 8th 2021 Member

@sgiehl

And on the PrivacyManager tests it looks like that before the All Websites entry was preselected in the site selector. Now that switched to a specific site. Would it be easy to change that?

Oh, I thought it didn't have an initial site set, but it is initialized in the controller. Looked into a bit, this is a different issue, though I'm not sure what... Setting data on the vue app isn't working, it doesn't propagate to the root component in the app. Will see if I can figure it out.

@diosmosis commented on December 8th 2021 Member

@sgiehl fixed it w/ an extra nextTick(). Not sure why this works, but it seems to.

@sgiehl commented on December 8th 2021 Member

@diosmosis there still seem to be some changes in the UI files that look unexpected. The usermanagers results changed, but imho they shouldn't have

@diosmosis commented on December 8th 2021 Member

@sgiehl: @peterhashair was faster by a couple minutes, but it looks like the change was a fix to some broken behavior, the first failure is for role_for.png, in the test we select for a site named "relentless": https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/UsersManager/tests/UI/UsersManager_spec.js#L68, but in the 4.x-dev screenshot the wrong site is selected. Will merge in a couple hours if there are no further comments.

@sgiehl commented on December 9th 2021 Member

@diosmosis I guess some ui changes might still be some sort of incorrect:
tests/UI/expected-screenshots/Menus_admin_loaded.png shouldn't have been updated. Guess that one will now fail on 4.x-dev. Maybe you can directly fix that there.
plugins/Dashboard/tests/UI/expected-screenshots/DashboardManager_removed.png did you check why the menu switched from being displayed fully, to be collapsible? Fo me it looks like the menu isn't updated anymore when a dashboard has been removed.

@diosmosis commented on December 9th 2021 Member

Maybe you can directly fix that there.
plugins/Dashboard/tests/UI/expected-screenshots/DashboardManager_removed.png did you check why the menu switched from being displayed fully, to be collapsible?

This works for me locally (when using tests/PHPUnit/proxy). It might be a race condition. The menu is updated, that's something I specifically had to fix (specifically due to a race condition w/ the jquery dashboard code not waiting for dashboard load to finish) though it might be possible the fix is in a different PR. I'll look a bit more to see if I can reproduce it.

This Pull Request was closed on December 9th 2021
Powered by GitHub Issue Mirror