@peterhashair opened this Pull Request on October 11th 2021 Contributor

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

@peterhashair commented on October 11th 2021 Contributor

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

@tsteur commented on October 12th 2021 Member

@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

@peterhashair commented on October 12th 2021 Contributor

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

@tsteur commented on October 13th 2021 Member

@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 commented on October 14th 2021 Contributor

@tsteur should work now 😬

@tsteur commented on October 15th 2021 Member

I've tested this in few different browsers and mostly worked well. It's pretty awesome! 🚀

I think it's expected it doesn't work on mobile?

There are still few issues though like this one below:

image

I see the same still in the UI test failures: eg in https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/50077/UIIntegrationTest_metric_tooltip.png

There is also https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/50077/UIIntegrationTest_goals_overview.png

Not sure if this is because of this PR? It doesn't seem related but then the test worked in another 4.x-dev build. Maybe it's a random build failure.

@peterhashair commented on October 15th 2021 Contributor

@tsteur it doesn't work on mobile, to work on mobile probably needs to restructure the entire table to a div, quite a big change. Let me know if we want to do that.

@tsteur commented on October 18th 2021 Member

to work on mobile probably needs to restructure the entire table to a div, quite a big change. Let me know if we want to do that.

Not needed for now 👍

Do you know why eg this screenshot test is failing now https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/50151/UIIntegrationTest_goals_overview.png ?

@sgiehl if you could give this one a test too that be great

@peterhashair commented on October 20th 2021 Contributor

@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 commented on October 21st 2021 Member

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 commented on October 22nd 2021 Member

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 commented on October 23rd 2021 Contributor

@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 commented on October 25th 2021 Member

👍 @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 commented on October 27th 2021 Contributor

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

Powered by GitHub Issue Mirror