Ensure javascript of a datatable only initializes itself #18386
Conversation
sgiehl
added
not-in-changelog
|
Seems the UI tests are passing so far. There are a couple of failures that are unrelated though. All those are already fixed on |
|
👍 |
There was a problem hiding this comment.
Generally sounds like a good fix 👍 Maybe this will also prevent random other issues.
Not sure if we should maybe merge this now into the 4.6 release or 4.7 release in case it breaks something worse? If I understand it right this has been an issue for a while?
|
@tsteur thanks for the feedback. I've applied some changes. |
|
@sgiehl I just tested this with a range and I still had the issue:
I double checked that there was no browser caching. |
|
@tsteur moved some js calculation to simpler css usage. hope that should fix it finally. At least I wasn't able to reproduce in local testing. |
| dataTableInCard.width(maxTableWidth); | ||
| dataTableInCard.css('max-width', maxTableWidth); | ||
| } else { | ||
| $domElem.width(maxTableWidth); | ||
| $domElem.css('max-width', maxTableWidth); | ||
| } | ||
|
|
||
| if (parentDataTable && parentDataTable.length) { | ||
| // makes sure dataTableWrapper and DataTable has same size => makes sure maxLabelWidth does not get | ||
| // applied in getLabelWidth() since they will have the same size. | ||
|
|
||
| if (dataTableInCard.length) { | ||
| dataTableInCard.width(maxTableWidth); | ||
| dataTableInCard.css('max-width', maxTableWidth); | ||
| } else { | ||
| parentDataTable.width(maxTableWidth); | ||
| parentDataTable.css('max-width', maxTableWidth); |
There was a problem hiding this comment.
Changing this caused the screenshot changes. Those are actually kind of expected, as the datatable will now only use the space available on the screen rather than exactly 1200px
| var tooltip = $(this).parents().filter(function() { | ||
| return jQuery.hasData(this) && $(this).data('ui-tooltip'); | ||
| }).tooltip('instance'); | ||
| var tooltip = $(this).parents('[piwik-widget]').tooltip('instance'); | ||
| if (tooltip) { | ||
| tooltip.disable(); | ||
| } | ||
| }, | ||
| close: function() { | ||
| var tooltip = $(this).parents().filter(function() { | ||
| return jQuery.hasData(this) && $(this).data('ui-tooltip'); | ||
| }).tooltip('instance'); | ||
| var tooltip = $(this).parents('[piwik-widget]').tooltip('instance'); |
There was a problem hiding this comment.
This should be a small improvement. The widget instance above actually holds the tooltip for the rows. Before it was iterating up all parents till the body to search for tooltips. This is actually useless as we know where the tooltip is.
There was a problem hiding this comment.
Nice, I couldn't reproduce the issue anymore and also worked in smaller resolutions. 👍 The only thing that would have been nice if in changed screenshots like https://github.com/matomo-org/matomo/pull/18386/files?short_path=606a79c#diff-606a79c8142da1db13cf950ec3f838ae72befbdcbc69bdc86874ede720ec3acc we could still see the entire output but I suppose that might not be easily doable?
I see some tests that also need updating. https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/51923/
Once tests are passing LGTM for me. If we could make the pivot test for example show the full width that be awesome but not required.
|
@tsteur I actually had a closer look at those tests. The full page of the pivot test looks like this: So the reason that it looks cut off, is actually that the resolution in tests is too low, to display the full size. Before it was displayed with a fixed width of 1200px, causing the full page to be scrollable, and that way it had enough space to be displayed fully. The screenshots that still needs to be updated, are located in a submodule. Will do that now, before merging this |
Description:
When a report containing some kind of datatable is loaded, the delivered code also includes some javascript to initialize the datatable (see
_dataTableJS.twig). We used to simply initialize all (new) datatables found on the page. This actually led to #18373. When switching the date range, all reports on the goals page are reloaded. In most cases the evolution chart is finished first. This chart already triggers a datatable initialize. In most cases the detail report below is already loaded but not yet rendered into the page. The javascript already tries to initialize the datatable, resulting in the weird behavior.This PR will change this, so only the report contained in the response (based on the reportid) will be initialized.
Locally that seems to work fine for me.
fixes #18373
Review