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

set table head sticky #18124

Merged
merged 40 commits into from Nov 2, 2021
Merged

set table head sticky #18124

merged 40 commits into from Nov 2, 2021

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Oct 11, 2021

Description:

Fixes: #3513

update table heads to sticky.

The sticky head will trigger on the first time load, not on screen resize, if you change screen size probably needs to reload the page. let me know if we want to have that on-screen resize.

On normal screen image

On the smaller device, I used to set the table body to scroll so the header stick on top, but doesn't seems really good, and failed lots of UI test, so when the table x-axis is overflowed sticky head won't work. Let me know if we want to enable this.

Review

Peter Zhang added 2 commits October 11, 2021 13:39
update table head to sticky
update on small screen
@peterhashair peterhashair changed the title update table head to sticky table head to sticky Oct 11, 2021
@peterhashair peterhashair changed the title table head to sticky set table head sticky Oct 11, 2021
Peter Zhang added 4 commits October 11, 2021 16:05
update table conditions
remove on smaller screen
update screen shot
@peterhashair peterhashair marked this pull request as ready for review October 11, 2021 20:26
@peterhashair peterhashair added this to the 4.6.0 milestone Oct 11, 2021
@peterhashair peterhashair added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Oct 11, 2021
@bx80 bx80 self-requested a review October 11, 2021 21:36
bx80
bx80 previously approved these changes Oct 11, 2021
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me. Probably not important but I noticed that with this change Firefox now shows a console warning "Relative positioning of table rows and row groups is now supported. This site may need to be updated because it may depend on this feature having no effect."

@peterhashair
Copy link
Contributor Author

@bx80 good spot, update a little bit, should stop the warning now.

@tsteur
Copy link
Member

tsteur commented Oct 12, 2021

@peterhashair the tests are failing see https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/49914/ this also happens for me locally

image
image

remove firefox warning
@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 12, 2021

@tsteur right, sorry the firefox warning fix broken it, revert it back. should be fine now.

@tsteur
Copy link
Member

tsteur commented Oct 13, 2021

@peterhashair I haven't tested yet but looking at the tests there might be still issues? eg https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/49937/UIIntegrationTest_dashboard2.png
image

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Oct 14, 2021
@peterhashair peterhashair added the Needs Review PRs that need a code review label Oct 19, 2021
@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 20, 2021

@sgiehl yeah, not the best appoach, I am trying to find those table looks ugly and not apply stick header, I was trying javascript find the width wider than the card, then don't apply stick header, but doesn't really work so well. Most people use scroll body instead of the scroll the whole page, but that means users need to hover the table to scroll. Here is a example https://codepen.io/peterhashair/pen/NWvRaMv?editors=101

@sgiehl
Copy link
Member

sgiehl commented Oct 21, 2021

Yes. Having the body scroll y combined with an scroll x in the table seems not to work with sticky headers. I'm not sure what the best approach would be, but having overlapping or completely hidden content doesn't sound like a good solution. We for sure could try to implement sticky headers using javascript, but not sure if that would be out of scope for now. ping @tsteur

@tsteur
Copy link
Member

tsteur commented Oct 22, 2021

How much time would it take to get a JS solution to work roughly?

As it's more complex than expected I'm thinking maybe we just don't do it.

@peterhashair
Copy link
Contributor Author

@tsteur I don't think it will take long, the first commit is a javascript solution it works, but it failed the UI test, maybe just need to alter the UI test

@tsteur
Copy link
Member

tsteur commented Oct 25, 2021

👍 @peterhashair sounds good to give it a try then. Let's restrict it to spend max 4 hours for now and then re-assess if needed.

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Oct 26, 2021
Peter Zhang and others added 3 commits October 27, 2021 10:10
@peterhashair peterhashair added the Needs Review PRs that need a code review label Oct 27, 2021
@peterhashair
Copy link
Contributor Author

@tsteur I think this is working, but needs to update a submodule screenshots

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new solution actually doesn't make sticky headers work in all cases. It kind of disables them for a table that is too wide and only works if the table fits. But imho that approach would be better than having nothing. ping @tsteur

plugins/CoreHome/javascripts/dataTable.js Outdated Show resolved Hide resolved
plugins/CoreHome/javascripts/dataTable.js Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Oct 28, 2021
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good to merge now. Will wait for the tests before merging. Maybe they need another update.

@sgiehl
Copy link
Member

sgiehl commented Nov 2, 2021

Failing tests are unrelated, so merging now.

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.

Column names could stick to the top of screen when scrolling down report tables
4 participants