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

Table : Fixed Header Feature #2214

Closed
wants to merge 13 commits into from
Closed

Table : Fixed Header Feature #2214

wants to merge 13 commits into from

Conversation

David-Moreira
Copy link
Contributor

Hello,
Here's the first version of the Table Sticky Header feature we've talked about on #1977
Tested on every provider successfully.

I have used overflow-y:overlay to solve the problem about scrollbar taking up table width.
However, I was taking a look at that css feature support. And it does not seem well supported. Specially since Firefox does not support it, and it's one of the major browsers.

@stsrki do you know an alternative css way of doing this? I was trying to avoid a js based solution.
I will also investigate more once I have more time.

https://caniuse.com/?search=CSS%20overflow%3A%20overlay

@stsrki
Copy link
Collaborator

stsrki commented Apr 19, 2021

Damn, it works really good in Chrome. But in Firefox it just goes beyond the table area :(

I will need to investigate for some other was of doing it.

@stsrki stsrki changed the title Table : Sticky Header Feature Table : Fixed Header Feature Apr 19, 2021
@stsrki
Copy link
Collaborator

stsrki commented Apr 19, 2021

So far I was trying a bunch of CSS only solutions but none seems to work as expected. If all fails we might go with JS route.

@David-Moreira
Copy link
Contributor Author

Damnit... I might try a bit more.
But I'm guessing we'll have to go the js route. Not sure when I have the time now.
Just let me know if you'd prefer to do it yourself. So we both don't work on the same thing. :P

@stsrki
Copy link
Collaborator

stsrki commented Apr 20, 2021

I'm currently doing some other PRs but I will definitely let you know if I go back to this. Also, I was looking and other frameworks and for example, MudBlazor has done it with a wrapper <div> around the table. Although it is a little dirty we might try the same.

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Apr 25, 2021

Hello,
So I started implementing the MudBlazor way, just noticed it's basically the solution we had already found out on #1933 :

image

We're gonna have trouble with multiple header rows, not sure if we can avoid js on those.
And we have to at least resync border and background styles.

I have updated the branch to the current state as I don't have yet time to keep going. Just let me know if you have any ideas in the meantime.

Edit: Working still just as fine on every provider.

@David-Moreira
Copy link
Contributor Author

Ok, so in the meantime I got back to this. I added the part where we handle multiple header rows.
What's missing?

  • Handle borders by provider.
  • Handle Backgroundcolor dynamically according to TableRow defined color.
  • There's an issue with multiple header row where you can see between the lines: View: Attached image
    image

Just let me know if you agree with this approach and I can go ahead and finish these three problems.

@stsrki
Copy link
Collaborator

stsrki commented Apr 26, 2021

I tested it and it seems to work fine. Although I'm not sure if we really need to have support for multiple header rows 🤔

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Apr 26, 2021

We do. We have two header rows in Datagrid, because of filter row, and we will use this feature on Virtualize.

@David-Moreira
Copy link
Contributor Author

I'll go ahead and do the rest of the corrections when I have time if you have no objections.

@stsrki
Copy link
Collaborator

stsrki commented Apr 26, 2021

You're right. We need multiple header rows,

OK, you can proceed with corrections and once you're done I can review it. Thanks!

@David-Moreira
Copy link
Contributor Author

Hello @stsrki
Made the corrections except
"- There's an issue with multiple header row where you can see between the lines: View: Attached image"

Seems like it "steals a pixel" both in Material and bootstrap, can you take a look see if you can easily solve it?

@stsrki
Copy link
Collaborator

stsrki commented May 1, 2021

For me, it seems to work on Bootstrap, although the borders look a little thick. And on Material, the borders are kinda transparent and as a result, the text from behind can be seen? Is that what you meant?

If I remove the border-top it seems to work but then it breaks other stuff. I will need to play more to make it work.

@David-Moreira
Copy link
Contributor Author

David-Moreira commented May 2, 2021

For me, it seems to work on Bootstrap, although the borders look a little thick. And on Material, the borders are kinda transparent and as a result, the text from behind can be seen? Is that what you meant?

If I remove the border-top it seems to work but then it breaks other stuff. I will need to play more to make it work.

I see. Thanks. Well if you're able to fix it, that's great! If you're too busy, I can eventually take a look again.

Edit: Yes, you can see through the in-between of the header rows.
image

This was referenced May 4, 2021
@stsrki stsrki closed this May 7, 2021
@David-Moreira David-Moreira deleted the dev094-table-stickyHeader branch May 7, 2021 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants