[DEV-17234] Update marketplace menu item #21456

michalkleiner · 2023-10-30T14:36:12Z

Description:

Ref. DEV-17234

@sgiehl can you check the approach here? It's based on Marc's investigation into how this might be possible to achieve and it seems to be working ok.

The approach is to allow any category to pass a vue component widget string that gets translated into a component when the menu is constructed.

The click event is reused from the category so the target widget loads in the righthand side area as expected.

I've also added the outline link to the UI demo view.

Remaining to do:

update icon font adding the shopping basket icon
use the basket icon in the menu button
update UI tests

Review

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

michalkleiner · 2023-11-01T07:23:42Z

@sgiehl can you please provide any feedback you may have on the approach? Is it ok to leave it like this or do we need to change it substantially (if so, how)?

sgiehl

Had a rough look through the code. The approach looks fine to me that way.
Left a couple of comments for things that came to my mind when looking through the changes...

core/Category/Category.php

plugins/Marketplace/lang/en.json

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

…n hover)

This reverts commit 5f37b02. We are not reverting the removal of the subcategory.

Partially reverts commit c351ec9.

…bcategories and an override widget

PM is ok with the menu items being removed from the quick access menu. Keyboard access to the new rich menu button works.

michalkleiner · 2023-11-08T13:20:50Z

tests/UI/expected-screenshots/QuickAccess_search_1.png

Confirmed with PM that the quick access items for Marketplace are OK to be removed as long as the new rich button in menu nav is accessible using a keyboard.

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

caddoo · 2023-11-09T00:07:54Z

plugins/API/tests/Unit/WidgetMetadataTest.php

@@ -48,6 +48,7 @@ public function test_buildWidgetMetadata_ShouldGenerateMetadata()
                'order' => 99,
                'icon' => '',
                'help' => '',
+                'widget' => null,


Probably a good idea to test this with an actual value at least once.

As you are missing a test for this logic:
'widget' => $category->getWidget() ?: null,

Yep, I can probably update one of the system tests where it returns the API response in XML format for comparison.

Otherwise the UI tests for the menu already cover the visual component override depending on this working.

caddoo · 2023-11-09T00:22:37Z

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

+        if (category.widget && category.widget.indexOf('.') > 0) {
+          const [widgetPlugin, widgetComponent] = category.widget.split('.');


Suggested change

if (category.widget && category.widget.indexOf('.') > 0) {

const [widgetPlugin, widgetComponent] = category.widget.split('.');

const widgetParts = category.widget.split('.');

if (category.widget && widgetParts.length === 2) {

const [widgetPlugin, widgetComponent] = widgetParts;

Just to prevent bugs/issues later when there are category.widget values of things like a.b.c

I can add a limit to the split function so it always produces two parts from the first two, or we use splat to catch all the remaining parts into the component.. or I can add a check to the setWidget call that would throw on invalid format inputs... basically I don't think it should be dealt with here in this way, but happy to discuss. It's basically a matter "if someone provides incompatible format widget notation, it won't work for them" and them is mostly us.

Maybe in the backend is better setWidget can be a run time error at least and stop any headaches going forward. A bit of defensive programming.
Otherwise it could be a value object with two parts plugin and component and a __toString() method that concats them in the correct format.

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

caddoo · 2023-11-09T08:43:12Z

I'm still a bit hesitant to approve this, but I did test it locally and it's all good. Maybe @sgiehl you wouldn't mind taking a look as you looked at the approach earlier in the PR

sgiehl

Overall looks good to me and works as expected. Left a minor comment for a possible improvement. Otherwise this looks good to merge.

sgiehl · 2023-11-09T15:48:34Z

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue

@@ -93,14 +100,21 @@
    </ul>
    <ul
      id="mobile-left-menu"
-      class="sidenav hide-on-large-only"
+      class="sidenav sidenav--reporting-menu-mobile hide-on-large-only"


Any reason for adding a new class name here instead of using the existing ones or maybe the id? Just wondering why the already available attributes shouldn't be enough to select the element in CSS. If the available classes aren't unique enough you could simply use the class reportingMenu of the surrounding div.

Also the double-dash class name looks really ugly. I know we have a couple other places where that is used, but imho we should maybe try to get rid of them instead of adding new ones.

Thanks for the comment @sgiehl!

Probably didn't need the new class, that's true. There's so many different ways of targeting elements in CSS. I don't think using the ID is the right way and I thought the sidenav wasn't unique to this element, and a combination of e.g. .sidenav.hide-on-large-only is also not the prettiest.
The -- double dash is a modifier syntax from the BEM methodology. Although there might be a couple of similar uses in other places, it's true it wasn't necessary here either.

We should perhaps standardise how we do CSS and front-end naming in general and then we can have a common and shared approach to new additions, otherwise it'll be a mix of things.

…ar (#21456) * Fix CS * Fix typo in UI demo example * Add styling and UI demo example for outline button (with reverse bg on hover) * Remove premium features widget * Allow category to have a custom widget for the reporting menu * Implement marketplace rich menu button * Remove surplus category config * Build vue files * Update unit tests with an empty category widget prop * Update system test expected files * Add type hints * Update UI test screenshots * Update expected API response xml * Partially revert "Remove premium features widget" This reverts commit 5f37b02. We are not reverting the removal of the subcategory. * built vue files * Restore Paid Plugins widget in system test expected files Partially reverts commit c351ec9. * Update expected test files * Use data attribute instead of CSS class * Add tabindex * Build vue files * built vue files * Let category load when there's exactly one subcategory or multiple subcategories and an override widget * Add support for keyboard submit * Build vue files * Update quick access search UI test screenshot PM is ok with the menu items being removed from the quick access menu. Keyboard access to the new rich menu button works. * Update built vue files --------- Co-authored-by: michalkleiner <michalkleiner@users.noreply.github.com>

…ar (4 Backport) (#21512) * [DEV-17234] Add more visually striking marketplace menu item in sidebar (#21456) * Fix CS * Fix typo in UI demo example * Add styling and UI demo example for outline button (with reverse bg on hover) * Remove premium features widget * Allow category to have a custom widget for the reporting menu * Implement marketplace rich menu button * Remove surplus category config * Build vue files * Update unit tests with an empty category widget prop * Update system test expected files * Add type hints * Update UI test screenshots * Update expected API response xml * Partially revert "Remove premium features widget" This reverts commit 5f37b02. We are not reverting the removal of the subcategory. * built vue files * Restore Paid Plugins widget in system test expected files Partially reverts commit c351ec9. * Update expected test files * Use data attribute instead of CSS class * Add tabindex * Build vue files * built vue files * Let category load when there's exactly one subcategory or multiple subcategories and an override widget * Add support for keyboard submit * Build vue files * Update quick access search UI test screenshot PM is ok with the menu items being removed from the quick access menu. Keyboard access to the new rich menu button works. * Update built vue files --------- Co-authored-by: michalkleiner <michalkleiner@users.noreply.github.com> * Update JS for 4.x * Update screenshot tests * Undo tests didn't need to change * Undo tests didn't need to change --------- Co-authored-by: Michal Kleiner <michal@innocraft.com> Co-authored-by: michalkleiner <michalkleiner@users.noreply.github.com>

michalkleiner commented Oct 30, 2023

View reviewed changes

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue Outdated Show resolved Hide resolved

michalkleiner commented Oct 30, 2023

View reviewed changes

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue Outdated Show resolved Hide resolved

michalkleiner force-pushed the dev-17234-marketplace-menu branch from e88fc15 to d0b14f2 Compare November 2, 2023 04:21

sgiehl requested changes Nov 2, 2023

View reviewed changes

core/Category/Category.php Outdated Show resolved Hide resolved

plugins/Marketplace/lang/en.json Show resolved Hide resolved

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue Outdated Show resolved Hide resolved

michalkleiner force-pushed the dev-17234-marketplace-menu branch from 49fda80 to 42a059e Compare November 3, 2023 01:35

michalkleiner added 11 commits November 7, 2023 17:57

Fix CS

5b2d1a2

Fix typo in UI demo example

5896a3a

Add styling and UI demo example for outline button (with reverse bg o…

8ea3554

…n hover)

Remove premium features widget

5f37b02

Allow category to have a custom widget for the reporting menu

5bdc4d1

Implement marketplace rich menu button

34a0513

Remove surplus category config

737ae90

Build vue files

6ba0354

Update unit tests with an empty category widget prop

00a5ce9

Update system test expected files

c351ec9

Add type hints

3b2863d

michalkleiner force-pushed the dev-17234-marketplace-menu branch from 42a059e to 3b2863d Compare November 7, 2023 05:33

michalkleiner and others added 6 commits November 7, 2023 23:41

Update UI test screenshots

c5327f1

Update expected API response xml

ec0376b

Partially revert "Remove premium features widget"

4b63aec

This reverts commit 5f37b02. We are not reverting the removal of the subcategory.

built vue files

d88098a

Restore Paid Plugins widget in system test expected files

addfb3b

Partially reverts commit c351ec9.

Update expected test files

112d069

michalkleiner force-pushed the dev-17234-marketplace-menu branch from 2c358b1 to 112d069 Compare November 7, 2023 21:23

michalkleiner and others added 5 commits November 8, 2023 15:16

Use data attribute instead of CSS class

73681c3

Add tabindex

3fbea38

Build vue files

3c4f780

built vue files

75a7232

Let category load when there's exactly one subcategory or multiple su…

70e48e0

…bcategories and an override widget

michalkleiner added 4 commits November 9, 2023 02:12

Add support for keyboard submit

b971906

Build vue files

846c14b

Update quick access search UI test screenshot

2ca1e38

PM is ok with the menu items being removed from the quick access menu. Keyboard access to the new rich menu button works.

Update built vue files

647b2e4

michalkleiner added c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review labels Nov 8, 2023

michalkleiner requested a review from a team November 8, 2023 13:15

michalkleiner commented Nov 8, 2023

View reviewed changes

plugins/CoreHome/vue/src/ReportingMenu/ReportingMenu.vue Show resolved Hide resolved

caddoo reviewed Nov 9, 2023

View reviewed changes

caddoo modified the milestone: 4.16.0 Nov 9, 2023

sgiehl reviewed Nov 9, 2023

View reviewed changes

sgiehl added this to the 5.0.0 milestone Nov 9, 2023

caddoo approved these changes Nov 9, 2023

View reviewed changes

caddoo merged commit da0f628 into 5.x-dev Nov 9, 2023

caddoo deleted the dev-17234-marketplace-menu branch November 9, 2023 21:21

caddoo mentioned this pull request Nov 9, 2023

Add more visually striking marketplace menu item in sidebar #21512

Merged

11 tasks

sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Dec 15, 2023

		if (category.widget && category.widget.indexOf('.') > 0) {
		const [widgetPlugin, widgetComponent] = category.widget.split('.');

-        if (category.widget && category.widget.indexOf('.') > 0) {
-          const [widgetPlugin, widgetComponent] = category.widget.split('.');
+        const widgetParts = category.widget.split('.');
+        if (category.widget && widgetParts.length ===  2) {
+          const [widgetPlugin, widgetComponent] = widgetParts;

Uh oh!

[DEV-17234] Update marketplace menu item #21456

[DEV-17234] Update marketplace menu item #21456

Uh oh!

Conversation

michalkleiner commented Oct 30, 2023

Description:

Review

Uh oh!

Uh oh!

Uh oh!

michalkleiner commented Nov 1, 2023

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caddoo commented Nov 9, 2023

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!