Skip to content
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

[Regression] Expand event name table column change the table size. #18728

Merged
merged 14 commits into from Feb 23, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Feb 2, 2022

Description:

Fixes: #18699

Problem: When expanding the event name table column change the table size.

It seems like the line below,$('tr:nth-child(1) td.label', domElem).length is increase by 1 when expand. That makes the table label smaller. Should just first parent label length.

return parseInt(labelWidth / $('tr:nth-child(1) td.label', domElem).length, 10);

Review

@peterhashair

This comment was marked as off-topic.

@tsteur
Copy link
Member

tsteur commented Feb 3, 2022

@peterhashair which part doesn't work there?

@sgiehl
Copy link
Member

sgiehl commented Feb 3, 2022

@peterhashair your approach doesn't look right. The reason why the table gets smaller or displays a table fully cut of, is that we have a javascript that tries to determine the expected / required width of the table. It then sets a max-width for the table. I guess if the calculation is fired too early the resulting values are incorrect. Maybe this is a regression of the migration of the widget loader to vue. But I guess we need to adjust the javascript

@peterhashair
Copy link
Contributor Author

@sgiehl right that make sense, will update

@tsteur
Copy link
Member

tsteur commented Feb 3, 2022

@peterhashair re the mobile issue: we don't want to create a table mobile view for now

@peterhashair
Copy link
Contributor Author

@sgiehl @tsteur sorry was trying the quick solution on demo.matomo.cloud browser CSS. Is there a way to generate some event name or can I copy the demo database to reproduce it locally

@tsteur
Copy link
Member

tsteur commented Feb 3, 2022

see https://matomo.org/docs/event-tracking/#implementing-event-tracking-with-matomo on how to track events. I believe this same issue applies though also to other reports, not just events.

I believe the visitor generator might generate events too.

update database table
@peterhashair peterhashair added this to the 4.8.0 milestone Feb 8, 2022
@peterhashair peterhashair added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Feb 8, 2022
@sgiehl
Copy link
Member

sgiehl commented Feb 9, 2022

Checked that on the events report. It seems when opening the first subtable the full table increases the width slightly. But opening further subtables don't change the width anymore and the automatically fit into the other tables width.
@peterhashair can you have a quick look if that can be fixed quickly. If it takes longer I would say this should be good enough as it is.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Feb 9, 2022
make table size stable
@peterhashair
Copy link
Contributor Author

@sgiehl should be stable size now. 😃

@peterhashair peterhashair added the Needs Review PRs that need a code review label Feb 13, 2022
@peterhashair
Copy link
Contributor Author

@sgiehl just checking is that one good to merge :)

@sgiehl
Copy link
Member

sgiehl commented Feb 14, 2022

@peterhashair So the current implementation still has the "issue" that the table might get a bit bigger when a subtable is opened, right?
For me that solution would be fine for now, as it at least improves it a lot. @tsteur is that fine, or shall we again check if it's possible to fix the increasing size as well?

@tsteur
Copy link
Member

tsteur commented Feb 14, 2022

Not sure if it's caused by this or if it's a different issue. I had a case where the table now got a lot wider than the widget itself. but only in some cases when I opened eg the first row. When I opened the second row it was scrolling within the available area. I couldn't reproduce it on an older version but maybe it only happens in specific cases:

Feb-15-2022.11-05-41.mp4

This happens sometimes that eventually the width gets too wide and the content overflows.

Apart from this I couldn't really reproduce the issue anymore I believe.

update columns
@peterhashair
Copy link
Contributor Author

@tsteur ah, right, I think this because the expanded has an extra column and the first column size changed. Did an update, it only recalculated once, I think should fix that problem.

This reverts commit c11aec8.
@peterhashair
Copy link
Contributor Author

peterhashair commented Feb 18, 2022

@sgiehl find another solution, just resize once, should fits nows :)

@sgiehl
Copy link
Member

sgiehl commented Feb 21, 2022

There is still the issue that smaller tables, like the report for events might slightly get wider when a subtable is opened.
I would suggest to leave that as is for now, at it seems not that easy to fix. The other issues seem fix.
@tsteur what do you think?

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Feb 21, 2022
@tsteur
Copy link
Member

tsteur commented Feb 21, 2022

That be fine for me 👍 . Does the table only get a bit wider or is the content overflowing? Overflowing be a bigger issue. If that's the case, could we create an issue for this and fix it as part of Matomo 4.9?

Either way be good to create a follow up issue that describes the problem and where it happens / when it happens. Then we can see if / when we fix it

@sgiehl
Copy link
Member

sgiehl commented Feb 23, 2022

@peterhashair could you create a follow-up issue and merge this one afterwards.

@peterhashair
Copy link
Contributor Author

@sgiehl sure

@peterhashair peterhashair merged commit e5e1ec6 into 4.x-dev Feb 23, 2022
@peterhashair peterhashair deleted the 18699-expand-bug branch February 23, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label shortened when opening multiple subtables
3 participants