-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add second set of data table action icons below the report title #22827
Conversation
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. |
91d09e8
to
80ee0a8
Compare
delete self.param.totalRows; | ||
delete self.param.totalRows; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually fixes the alignment in IDE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transitions are not within the data table wrapper, so they don't get the top controls.
606f240
to
a8e4e9a
Compare
This should be resolved. |
See internal Jira card for more details on the context of the change. |
Remaining failing UI tests come from submodules. |
I've been reviewing the screenshots and found a few that don't look right. All the This also applies to the Looking at I'll stop reviewing now until those are corrected, as i think fixing them once will fix a lot of the screenshots. |
Thanks for the feedback @caddoo.
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
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? |
@michalkleiner there are still a couple of inconsistencies. For example if you search for something and the table then has no results: report_search.mp4And another problem I've spotted is, that the datatable setting do not always have the same options: report_settings.mp4 |
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. |
The flattening option is not added, it is removed from the table: matomo/plugins/CoreHome/javascripts/dataTable.js Lines 1262 to 1270 in 0973713
So the top action row is correct, and the bottom one is not, because there is no loop over both |
Plugin PRs created |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
# Conflicts: # tests/UI/expected-screenshots/Comparison_multi_row_evolution.png # tests/UI/expected-screenshots/IncompletePeriodVisualisation_visitors_overview.png
@matomo-org/core-reviewers please have another look, the merge conflict was resolved, there are no errors in UI tests, the failing ones are from submodules that have their respective PRs prepared to be merged after this. |
Description:
Ref. DEV-13900
Review