Add second set of data table action icons below the report title #22827

michalkleiner · 2024-12-05T12:52:08Z

Description:

Ref. DEV-13900

Review

michalkleiner · 2024-12-05T20:29:37Z

So far found a weird behaviour where the top actions don't display the settings icon when the report doesn't show data, need to dig into that.

michalkleiner · 2024-12-10T08:15:11Z

plugins/CoreHome/javascripts/dataTable.js

-					delete self.param.totalRows;
+					          delete self.param.totalRows;


This actually fixes the alignment in IDE

michalkleiner · 2024-12-10T09:50:50Z

...nsitions/tests/UI/expected-screenshots/Transitions_transitions_report_period_not_allowed.png

Transitions are not within the data table wrapper, so they don't get the top controls.

michalkleiner · 2024-12-10T10:32:29Z

So far found a weird behaviour where the top actions don't display the settings icon when the report doesn't show data, need to dig into that.

This should be resolved.

michalkleiner · 2024-12-10T11:01:41Z

See internal Jira card for more details on the context of the change.

michalkleiner · 2024-12-10T11:43:06Z

Remaining failing UI tests come from submodules.

caddoo · 2024-12-11T00:41:52Z

I've been reviewing the screenshots and found a few that don't look right.

All the ActionsDataTable_* have the icons bleed out over the top border.

This also applies to the GoalsTable_* as well and a lot more. I won't spend the time listing them all.

Looking at Goals_action_goals_visualization_page_urls, Goals_action_goals_visualization_page_urls, Goals_goals_by_entry_page_titles, Goals_goals_by_entry_pages it looks as though there where no controls before and now they are there, is that expected?

I'll stop reviewing now until those are corrected, as i think fixing them once will fix a lot of the screenshots.

michalkleiner · 2024-12-11T03:22:22Z

Thanks for the feedback @caddoo.

I've been reviewing the screenshots and found a few that don't look right.

All the ActionsDataTable_* have the icons bleed out over the top border.

This also applies to the GoalsTable_* as well and a lot more. I won't spend the time listing them all.

They actually don't spill over, they are at the very edge with 0 pixels gap. My understanding is that is due to how the report screenshot is taken, not that there would be anything missing. However we do use -10px top margin to lower the gap between the actions and the title, so I can look into updating the styles to only do that when the card title is a sibling element. That should address that.

Looking at Goals_action_goals_visualization_page_urls, Goals_action_goals_visualization_page_urls, Goals_goals_by_entry_page_titles, Goals_goals_by_entry_pages it looks as though there where no controls before and now they are there, is that expected?

Again, this is due to how the screenshots are taken and where the mouse is during that event since the bottom actions were visible on hover. If the mouse is moved outside of the report or the report is not being interacted with, the hover effect is not there and the bottom action are not visible. The change makes both bottom and top controls visible at all times, which is why it is newly visible both at the bottom and at the top. Does that answer your question?

sgiehl · 2024-12-11T16:57:08Z

@michalkleiner there are still a couple of inconsistencies. For example if you search for something and the table then has no results:

report_search.mp4

And another problem I've spotted is, that the datatable setting do not always have the same options:

report_settings.mp4

michalkleiner · 2024-12-11T21:08:12Z

Thanks for your testing @sgiehl. The inconsistency around the top controls being hidden came from it being hidden when there's no data initially, but it didn't occur to me that it would also apply when searching, so I guess I'll need to check if we can differentiate those two scenarios - initial table empty and empty search results.

For the latter, I have no idea why that would be happening, unless someone was adding the options via JS. I'll investigate, but any hints would be appreciated.

mneudert · 2024-12-11T21:24:01Z

The flattening option is not added, it is removed from the table:

matomo/plugins/CoreHome/javascripts/dataTable.js

Lines 1262 to 1270 in 0973713

    
           if ((typeof self.numberOfSubtables == 'undefined' || self.numberOfSubtables == 0) 
        
               && (typeof self.param.flat == 'undefined' || self.param.flat != 1) 
        
           ) { 
        
               // if there are no subtables, remove the flatten action 
        
               const dataTableActionsVueApp = $('[vue-entry="CoreHome.DataTableActions"]', domElem).data('vueAppInstance'); 
        
               if (dataTableActionsVueApp) { 
        
                 dataTableActionsVueApp.showFlattenTable_ = false; 
        
               } 
        
           }

So the top action row is correct, and the bottom one is not, because there is no loop over both $('[vue-entry="CoreHome.DataTableActions"]', domElem).

… controls

… all

michalkleiner · 2024-12-12T21:09:28Z

@sgiehl I've addressed both issues you raised, we now can identify if there are no data in the report based on search, or whether it's an empty report without search. With search, we display the top controls if they were visible before.

For the settings option, thanks to @mneudert's suggestion this is also fixed and the flattening option is correctly removed from both controls.

@caddoo I've checked reports without titles to see if the action icons bunched up against the top border of the screenshot is an issue and in the UI itself I couldn't find anywhere where this would be a problem, so I think it comes down to how the screenshots are taken.

michalkleiner · 2024-12-12T21:09:53Z

I've rebased the branch on latest 5.x-dev to prevent UI screenshots braking on there being a new version of Matomo available.

plugins/CoreHome/javascripts/dataTable.js

# Conflicts: # plugins/CustomDimensions/tests/UI/expected-screenshots/CustomDimensions_report_actions_segmented_visitorlog.png # plugins/Dashboard/tests/UI/expected-screenshots/Dashboard_dashboard3.png # plugins/Live/tests/UI/expected-screenshots/Live_visitor_log.png # plugins/Live/tests/UI/expected-screenshots/Live_visitor_log_page_next.png # plugins/Referrers/tests/UI/expected-screenshots/ReferrersPages_search_engines_keywords.png # tests/UI/expected-screenshots/Comparison_normal_table.png # tests/UI/expected-screenshots/Comparison_normal_table_no_periods.png # tests/UI/expected-screenshots/Comparison_normal_table_no_segments.png # tests/UI/expected-screenshots/Comparison_row_evolution.png # tests/UI/expected-screenshots/Comparison_segmented_visitorlog.png # tests/UI/expected-screenshots/Comparison_subtables_loaded.png # tests/UI/expected-screenshots/Comparison_subtables_paginate.png # tests/UI/expected-screenshots/JSTracker_visitor_log.png # tests/UI/expected-screenshots/UIIntegrationTest_actions_outlinks_vlog.png # tests/UI/expected-screenshots/UIIntegrationTest_ecommerce_log_segmented.png # tests/UI/expected-screenshots/UIIntegrationTest_segmented_visitorlog.png # tests/UI/expected-screenshots/UIIntegrationTest_visitors_devices.png # tests/UI/expected-screenshots/UIIntegrationTest_visitors_software.png # tests/UI/expected-screenshots/UIIntegrationTest_widgetize_ecommercelog.png # tests/UI/expected-screenshots/UIIntegrationTest_widgetize_visitor_log.png # tests/UI/expected-screenshots/ViewDataTableTest_11_flattened.png # tests/UI/expected-screenshots/ViewDataTableTest_12_aggregate_shown.png # tests/UI/expected-screenshots/ViewDataTableTest_dimension_columns.png # tests/UI/expected-screenshots/ViewDataTableTest_dimension_search.png # tests/UI/expected-screenshots/ViewDataTableTest_flatten_search.png # tests/UI/expected-screenshots/ViewDataTableTest_related_report_click.png # tests/UI/expected-screenshots/ViewDataTableTest_subtables_loaded.png

michalkleiner · 2024-12-18T10:35:54Z

There is a few other UI screenshots updated as a leftover from the icons update.

mneudert · 2024-12-18T18:25:29Z

tests/UI/specs/EvolutionGraph_spec.js

@@ -197,7 +197,7 @@ describe("EvolutionGraph", function () {
        });
        await page.reload();
        await showDataTableFooter();
-        await page.click('.activatePeriodsSelection');
+        await page.click('.activatePeriodsSelection:last');


Suggested change

await page.click('.activatePeriodsSelection:last');

await (await page.jQuery('.activatePeriodsSelection:last')).click();

(or something like that)

Otherwise the UI tests will keep failing:

Evaluation failed: DOMException: Failed to execute 'querySelector' on 'Document': '.activatePeriodsSelection:last' is not a valid selector.

Or we could treat those "single module widget iframes" like the dashboard widgets and hide the duplicate controls. Adding and not isWidget to the showTableActionsInHeader template variable should do that well enough, and we can use the old selector again.

I went with the and is not isWidget approach, let's see how that behaves.

Hmm, that didn't make any difference.

tests/UI/specs/EvolutionGraph_spec.js

mneudert · 2024-12-18T18:54:33Z

plugins/CoreHome/templates/_dataTable.twig

+of moving the table actions to the top of the report for all reports, at which point the config for the report
+can be renamed to show_header and show_header_icons
+#}
+{% set showTableActionsInHeader = properties.show_footer and properties.show_footer_icons %}


Suggested change

{% set showTableActionsInHeader = properties.show_footer and properties.show_footer_icons %}

{% set showTableActionsInHeader = properties.show_footer and properties.show_footer_icons and not isWidget %}

I have not fully checked what more side effects this would have (other than the mentioned single widgets in EvolutionGraph_spec), but this would remove the slight layout change in the Dashboard_spec caused by some other CSS:

matomo/plugins/Live/stylesheets/live.less

Lines 419 to 421 in eed6ea9

&:first-child {

padding-top:0;

}

mneudert · 2024-12-18T19:05:29Z

plugins/CoreHome/templates/_dataTable.twig

@@ -50,6 +57,14 @@
        {% if error is defined %}
            <div vue-entry="CoreHome.Alert" severity="danger">{{ error.message }}</div>
        {% else %}
+            {% if showTableActionsInHeader %}
+                <div class="row dataTableHeaderControls">


The Live_spec tests have some selectors that get tripped by this new element:

matomo/plugins/Live/tests/UI/Live_spec.js

Line 34 in 9a3ef94

var report = await page.$('.dataTableVizVisitorLog .card.row:first-child');

This would now need to be something like this:

var report = await page.jQuery('.dataTableVizVisitorLog .card.row:eq(1)');

And there are probably more selectors in that spec that need to be updated accordingly.

This shouldn't trip on this, as the Visits Log does not have a card on the top level, so the first row for the table controls is not within a card.

Updated the selectors

michalkleiner · 2024-12-20T15:30:58Z

Plugin PRs created

michalkleiner force-pushed the dev-13900 branch from 91d09e8 to 80ee0a8 Compare December 8, 2024 23:08

michalkleiner commented Dec 10, 2024

View reviewed changes

michalkleiner force-pushed the dev-13900 branch from 606f240 to a8e4e9a Compare December 10, 2024 10:01

michalkleiner added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Design / UI For issues that impact Matomo's user interface or the design overall. labels Dec 10, 2024

michalkleiner added this to the 5.3.0 milestone Dec 10, 2024

michalkleiner marked this pull request as ready for review December 10, 2024 11:01

michalkleiner added the Needs Review PRs that need a code review label Dec 10, 2024

michalkleiner requested a review from a team December 10, 2024 11:02

michalkleiner changed the title ~~Add second set of data table action icons below report title~~ Add second set of data table action icons below the report title Dec 10, 2024

michalkleiner added 10 commits December 13, 2024 09:48

WIP

be6ebb9

Move top actions below the title

580977b

Use random id for search action

ff9fc8c

Allow for data table actions specific placement

309029c

Ensure search is functional and correctly focusing used input

575be25

Revert random id var name

f6de4b7

Remove unused styles

ac2c26a

Use custom variable for header actions

d51ff23

Hide top controls on dashboard

f7bd1ba

Hide top controls when report doesn't have data

8a7af46

michalkleiner added 3 commits December 13, 2024 09:48

Update UI tests

a68e3bc

Ensure the flatten report setting is removed from both top and bottom…

8e875f4

… controls

Ensure empty search results table displays top controls vs no data at…

6a7fcfc

… all

michalkleiner force-pushed the dev-13900 branch from 8867894 to 6a7fcfc Compare December 12, 2024 20:48

Merge branch '5.x-dev' into dev-13900

193daf6

mneudert requested changes Dec 16, 2024

View reviewed changes

plugins/CoreHome/javascripts/dataTable.js Outdated Show resolved Hide resolved

michalkleiner added 4 commits December 17, 2024 17:55

Ensure search fields correctly calculate their width

a48064a

Update UI test screenshots

229dd3a

Update UI test unrelated to the PR but the icons change

8be965b

michalkleiner requested review from mneudert and a team December 18, 2024 10:35

mneudert requested changes Dec 18, 2024

View reviewed changes

michalkleiner added 7 commits December 19, 2024 13:49

Remove hover to display table controls in UI tests as always visible now

3f2b0a3

Adjust for tests

f73ed92

Fix UI tests

bc58b67

Update UI test screenshots

1008eaa

Move mouse out of view to prevent unwanted hover

6dc8d38

Revert 'not is Widget' addition

da3de87

Update UI test screenshots

7585e39

Merge branch '5.x-dev' into dev-13900

855edcc

michalkleiner requested review from mneudert and a team December 20, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add second set of data table action icons below the report title #22827

Add second set of data table action icons below the report title #22827

michalkleiner commented Dec 5, 2024

michalkleiner commented Dec 5, 2024

michalkleiner Dec 10, 2024

michalkleiner Dec 10, 2024

michalkleiner commented Dec 10, 2024

michalkleiner commented Dec 10, 2024

michalkleiner commented Dec 10, 2024

caddoo commented Dec 11, 2024

michalkleiner commented Dec 11, 2024

sgiehl commented Dec 11, 2024

michalkleiner commented Dec 11, 2024

mneudert commented Dec 11, 2024

michalkleiner commented Dec 12, 2024

michalkleiner commented Dec 12, 2024

michalkleiner commented Dec 18, 2024

mneudert Dec 18, 2024

michalkleiner Dec 19, 2024

michalkleiner Dec 19, 2024

mneudert Dec 18, 2024

mneudert Dec 18, 2024

michalkleiner Dec 19, 2024

michalkleiner Dec 20, 2024

michalkleiner commented Dec 20, 2024

	await page.click('.activatePeriodsSelection:last');
	await (await page.jQuery('.activatePeriodsSelection:last')).click();

	{% set showTableActionsInHeader = properties.show_footer and properties.show_footer_icons %}
	{% set showTableActionsInHeader = properties.show_footer and properties.show_footer_icons and not isWidget %}

Add second set of data table action icons below the report title #22827

Are you sure you want to change the base?

Add second set of data table action icons below the report title #22827

Conversation

michalkleiner commented Dec 5, 2024

Description:

Review

michalkleiner commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Dec 10, 2024

michalkleiner commented Dec 10, 2024

michalkleiner commented Dec 10, 2024

caddoo commented Dec 11, 2024

michalkleiner commented Dec 11, 2024

sgiehl commented Dec 11, 2024

michalkleiner commented Dec 11, 2024

mneudert commented Dec 11, 2024

michalkleiner commented Dec 12, 2024

michalkleiner commented Dec 12, 2024

michalkleiner commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Dec 20, 2024