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

Jump to bottom

Fix overflow handling of datatables #16685

Merged
merged 3 commits into from Dec 10, 2020
Merged

Fix overflow handling of datatables #16685

merged 3 commits into from Dec 10, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Nov 9, 2020

Description:

With introducing the datatable scroller in #14594, there actually shouldn't be any further need of checking if the table has enough space, as it always fits. Guess adding a overflow-y was needed to ensure the overlays were at least visible with scrolling.
The current handling actually always displays a vertical scroll bar as soon as the table would be too wide (without the datatable scroller), causing all overlays to be displayed in the report instead of overlaying it:

Chrome:
image

Firefox & IE:
image

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl 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 9, 2020
@sgiehl sgiehl added this to the 4.0.0-RC milestone Nov 9, 2020
@tsteur tsteur modified the milestones: 4.0.0-RC, 4.1.0 Nov 9, 2020
@tsteur
Copy link
Member

tsteur commented Nov 9, 2020

@sgiehl I'm not sure what this fixes? Is there an issue for it?

@sgiehl
Copy link
Member Author

sgiehl commented Nov 10, 2020

There is no issue yet afaik. Came across that issue when testing the page performance stuff. You can easily reproduce it with any dashboard widget that would be wider than the available space. You can use the page url widget for example in a small dashboard column. It will always show the vertical scrollbar, even though that isn't actually needed. When you click one of the footer icons, the overlays aren't showing correctly because of that.

@diosmosis
Copy link
Member

Can't reproduce the issue, but I didn't notice any problem w/ the change so I guess it's good to at least remove unneeded code. It's possible I didn't test the exact place where this happens though...

@sgiehl
Copy link
Member Author

sgiehl commented Nov 23, 2020

@diosmosis Steps to reproduce:
Go to the dashboard. Add the pages widget. The widget should always show an vertical scrollbar. Click the limit selection in the footer, this should kind of break the layout (which should not happen with this changes)

@diosmosis
Copy link
Member

@sgiehl was able to reproduce, he fix works 👍

@sgiehl sgiehl merged commit 14da255 into 4.x-dev Dec 10, 2020
@sgiehl sgiehl deleted the removejs branch December 10, 2020 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis approved these changes

Assignees
No one assigned
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgiehl @tsteur @diosmosis