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
set table head sticky #18124
Conversation
update table head to sticky
update on small screen
update table conditions
remove on smaller screen
update screen shot
There was a problem hiding this 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."
update table warnings
@bx80 good spot, update a little bit, should stop the warning now. |
@peterhashair the tests are failing see https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/49914/ this also happens for me locally |
remove firefox warning
@tsteur right, sorry the firefox warning fix broken it, revert it back. should be fine now. |
@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 |
test new setup
remove js, sticky on pure css
@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 |
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 |
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. |
@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 |
👍 @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. |
update to js solution
update table
@tsteur I think this is working, but needs to update a submodule screenshots |
There was a problem hiding this 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
add on load blinds, remove console.log
update screen shot
There was a problem hiding this 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.
Failing tests are unrelated, so merging now. |
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
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