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

[docs] Table virtualized #7450

Closed
damianobarbati opened this issue Jul 17, 2017 · 55 comments
Closed

[docs] Table virtualized #7450

damianobarbati opened this issue Jul 17, 2017 · 55 comments
Labels
component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@damianobarbati
Copy link

@oliviertassinari would you consider to add a virtual container to the tbody to increase performance when rendering long lists without pagination?
I'll take a stab at this if you could just point me in the right direction (react-virtualized?)

@oliviertassinari oliviertassinari added component: table This is the name of the generic UI component, not the React module! v1 labels Jul 17, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 17, 2017

We hope that the current API allow simple integration with react-virtualized. We are open to do any change to the implementation in order to make the integration simpler.

@oliviertassinari
Copy link
Member

Having an demo example in the docs would be awesome.

@oliviertassinari

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title Table performance (@next) [docs] Table performance Jul 17, 2017
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 17, 2017
@oliviertassinari oliviertassinari changed the title [docs] Table performance [docs] Table virtualized Jul 17, 2017
@damianobarbati
Copy link
Author

damianobarbati commented Jul 19, 2017

@oliviertassinari I made it without react-virtualized because it was a mess and it didn't play nice with mui components.
Furthermore, my current solution is a pure-css fixed thead and scrollable tbody with dynamic table height and dynamic columns width (tested on Safari as well).
The always-on-top thead with a scrollable tbody is a must for data grids.

I'm adding scroll debouncing, tweaking performance and fixing some glitch but it's working great: I'm "virtually" rendering about ~2400rows, sooo smooth.

out

Questions on Table components:

  1. how to use ref with MUI components? I'm auto-computing TableThead height: problem is that
<TableHead ref={ ref => this.tableThead = ref }>

is not returning the actual DOM container element of TableThead but the React instance (native behaviour of React when applying ref on a not-DOM element) because that's something that should be applied in the TableThead component itself; other components may need this, so why not having a refHandler on every MUI component which applies the given callback in the first rendered element? (I hope I managed to explain this out @_@)
My current solution is a weird

const theadHeight = ReactDOM.findDOMNode(this.tableThead).clientHeight; // eslint-disable-line react/no-find-dom-node

which means using the deprecated findDomNode and disabling eslint as well.

  1. why is border-bottom applied on cells instead of rows?

  2. what about having a stripe={true} prop to automatically stripe TableBody rows?

  3. why does .MuiIconButton.root have a zIndex style property? 🤔

I hope to publish something stable about this soon so you can give a look it that's something which can be adopted in a general way.

Sorry to bother this much but I'm heavily using mui@next so I'm spotting a lot of use-cases!

@oliviertassinari
Copy link
Member

I made it without react-virtualized because it was a mess and it didn't play nice with mui components.

Oh, that's a shame on us. Any idea what we could do to make is possible?

  1. You can try the innerRef to get a ref on the TableHead. ref will get you a ref on the withStyles(TableHead) component. If that doesn't work we could expose a rootRef property. That's a pattern we are already using.

  2. No clue

  3. What do you mean by stripe?

  4. I have removed it in [IconButton] Remove z-index #7467

@damianobarbati
Copy link
Author

damianobarbati commented Jul 19, 2017

Oh, that's a shame on us. Any idea what we could do to make is possible?

Actually is a mix of react-virtualized own containers, native table/thead/tbody DOM elements and the need of having a scrollable tbody which caused the problem.
Too much work was needed with it: simply making an ad-hoc solution for MuiTable was a breeze.

  1. exposing a rootRef would be great (assuming TableThead main element in the render method is the Thead DOM element itself)

  2. should border-bottom be moved to tr then? Should I move this into a dedicated issue?

  3. I mean striped rows: easy to achieve with like tr:nth-child(odd) { backgroundColor: f2f2f2 but since we already have the handy hover={true} coloring why not having the an handy striped={true}? :)

  4. great!

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2017

  1. 👍 Sounds like a plan, here is an example, a PR is welcomed.
  2. Bootstrap is doing the same, I think that it's providing more flexibility by using it on the TableCell.
  3. Because it's not in the specification and it can be simply implemented on userland?
  4. 👍

@damianobarbati
Copy link
Author

damianobarbati commented Jul 19, 2017

  1. I'm not sure about simply having ref={rootRef} on every component.
    Devs who are using it are probably (mostly) in need to have control over the DOM.
    In your example <FormControl ref={rootRef} ...> will again return a reference to the React component (because FormControl is not a DOM element) and will have the developer use the workaround I had to use.
    Maybe we should have the rootRef behave like the following:

Is the container of the rendered component a DOM element?

  • if yes then apply ref={rootRef} on it
  • otherwise apply rootRef={rootRef} on it => which is going to propagate down to the first DOM element: this way coder will always have the real DOM reference

The catch-all solution I can think about is having muiRef and rootRef properties where the former applies the callback to the "mui component" container (if any) and the latter applies the callback as I said before.
This way Developer can easily have both/either MUI ref and DOM ref (whatever he needs, whenever he needs).
Do you think this make sense?

  1. really? @_@

  2. ops! I always forget about the material specs

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2017

  1. Oh, I haven't thought about that case. The issue you are referring to is the following:
  • Component A takes a ref and a rootRef property
  • React intercept ref and apply it on the element
  • Component B receive a rootRef property and apply it as a ref on the root Component C
  • React intercept this ref and apply it on the element

But what if Component C also have a rootRef property?
Component A can be on user-land, Component B can be the TextField and Component C can be the FormControl.

So my answer, at first we introduced rootRef as a simple helper. We haven't though about industrialization. So, I think that the current behavior is great as deterministic, simple and without the limitation you are describing, if people are interested in accessing the underlying dom element, they can use the findDOMNode API.

@kgregory
Copy link
Member

kgregory commented Jan 2, 2018

@damianobarbati Was react-virtual-list the experiment with Table and virtualization mentioned here?

I'm guessing listComponent = TableBody, itemComponent = TableRow?

@techniq
Copy link

techniq commented Jan 3, 2018

I've started this project that uses react-virtualized with Material 1.0 you might be interested in.

https://github.com/techniq/mui-table

It's still a work in progress (see https://github.com/techniq/mui-table/blob/master/TODO.md) but might be useful to you. Docs are lacking but take a look at the stories to get a feel for how to use it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 3, 2018

@techniq This looks promising 😄 ! I think it would be great to revamp the table documentation section at some point to have:
- https://github.com/DevExpress/devextreme-reactive (commercial license)

We can also add it to the https://material-ui.com/discover-more/related-projects/#material-ui-specific-projects section.
So, let us know when the project is more mature. I think that we can accept a pull request to promote the project :).

@eyn
Copy link
Contributor

eyn commented Jan 5, 2018

I have done a PR for react-virtual-list here clauderic/react-tiny-virtual-list#30 which allows its usage with material-ui. I struggled to get react-virtual to work and create valid HTML as it has div's hard coded.

cc: @kgregory

@kgregory
Copy link
Member

kgregory commented Jan 5, 2018

@eyn nice work! I just finished up an implementation of an infinite-scrolling table using react-virtualized and material-ui. I hope to throw a gist together soon. When I do I will link it here for any future visitors of this issue and possibly as an example in the docs.

@oliviertassinari oliviertassinari added this to the post v1 milestone Feb 11, 2018
@Neofox
Copy link

Neofox commented Mar 22, 2018

@kgregory did you finished your gist? I'm still looking for a basic solution for virtualized Material UI table and it's not easy to find... It would be a great help!

Thanks for your work!

@kgregory
Copy link
Member

@Neofox No, sorry. I never got around to it, but your reply is a nice reminder. I will try to find time to put something together.

@rolandjitsu
Copy link
Contributor

@oliviertassinari your example in one of the first comments that supposedly shows how to use the material list with the react virtualized list is not working. Do you still have that example laying around?

@techniq
Copy link

techniq commented Oct 17, 2018

@mastertinner While I still use it (when virtualization is needed), I also use my newer mui-table when I don't need a virtualization, and can leverage the dom (using table, tr, td, or css grid for layout, removing the need for explicit column widths, etc) and enables some advanced patterns like multiple position: sticky headers (like this)

@IssueHuntBot
Copy link

@rogerstorm funded this issue with $30. See it on IssueHunt

@joshwooding
Copy link
Member

Decided to play around with this today, I do not recommend 😭 Here's what I've produced so far:
virtualize

Please excuse the lag, my laptop isn't great 👎 It uses Material-UI Components and react-virtualized components

@worudso
Copy link

worudso commented Nov 21, 2018

@joshwooding I've done exact same thing what you've done, virtualized + fixed header(with scrollbar on body only). If your implementation uses two seperate <Table>s, then I think you got the same problems I've found.

  • material's alignment props (e.g. numeric) doesn't work, because you might use a component like <div> for layout inside <TableCell>.
  • You might give css properties for td and tr to work virtualized correctly. There's no way to give height or max-height to td and tr and material's <TableCell> has its own padding, margin things so it makes it hard to fix row's height.

Well, the point is a component inside <TableCell> is inevitable. I wonder if you solved this problem in a elegant way. If not, I think virtualized + fixed header(with scrollbar on body only) is hard to be a small(as small as its function),well defined component.

@joshwooding
Copy link
Member

@worudso Currently it uses only one <Table> I need to find a way to pass numeric down but I might be able to get that working. It’s definitely not a small Component and my code’s still a bit all over the place but I’m going to work on it some more and see if I can refine it

@kgregory
Copy link
Member

kgregory commented Nov 21, 2018

I've created a sandbox that demonstrates using material-ui and react-virtualized to produce a virtualized table. This particular example uses react-virtualized to size the Table to the height of the window.

The MuiVirtualizedTable component from the example is also available in this gist.

MuiVirtualizedTable can accept any prop supported by a Table. The rowClassName is not directly applied, but is instead applied in addition to other styles defined in the MuiVirtualizedTable.

The columns prop expects an array of objects that is used when rendering cells for each column. Each object can accept any prop supported by Column.

My apologies for taking so long. I've been busy.

@Janpot
Copy link
Member

Janpot commented Nov 22, 2018

FYI, there's now also react-window which is supposed to be react-virtualized successor. In case anyone wanted to try that one out.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 22, 2018

@kgregory Thank you for sharing the codesandbox.

@joshwooding Great demo. It's great to have a fixed header :). I hope you will be able to complete the implementation! Regarding the numeric forwarding, what's preventing you from applying directly on the cells? react-window vs react-virtualized

@joshwooding
Copy link
Member

@oliviertassinari I've set myself the goal of using the Table component's API and right now it doesn't look like you can pass custom props via the Column component. There are probably ways to do it without using the react-virtualized way though

@oliviertassinari
Copy link
Member

@joshwooding Did you try https://github.com/bvaughn/react-virtualized/blob/master/docs/Table.md#headerrowrenderer?

@joshwooding
Copy link
Member

@oliviertassinari I've tried using HeaderRowRenderer but it looks like you can't pass custom props through it without manipulating the existing props since this is what is called to create the columns to pass through to the headerRenderer:

const renderedHeader = headerRenderer({ columnData, dataKey, disableSort, label, sortBy, sortDirection, });

@joshwooding
Copy link
Member

@oliviertassinari For a fixed height table you can just edit @kgregory's demo slightly: https://codesandbox.io/s/6vo8y0929k

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 1, 2018

@joshwooding Looks great! I can't think of anything missing to make it an official demo.

@IssueHuntBot
Copy link

@issuehuntfest has funded $60.00 to this issue. See it on IssueHunt

@joshwooding
Copy link
Member

@oliviertassinari I'll work on implementing this :)

@IssueHuntBot
Copy link

@joshwooding has submitted a pull request. See it on IssueHunt

@IssueHuntBot
Copy link

@oliviertassinari has rewarded $81.00 to @joshwooding. See it on IssueHunt

  • 💰 Total deposit: $90.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $9.00

@kgregory
Copy link
Member

@oliviertassinari has rewarded $81.00 to @joshwooding. See it on IssueHunt

Well deserved. Nice work @joshwooding!

@aguywithcode
Copy link

I have a similar need but for the List component...would the same approach work for making the list work?

@oliviertassinari
Copy link
Member

@mdizzy Yes, we could add a similar demo for the list.

@MastroLindus
Copy link

I am trying right now to make a list virtual with react-window (as the creator suggested to use it in place of virtualized if you don't need the full API, but in the end the API for a list doesn't really change that much)

So far I have two big issues:
1)List items style is broken, especially for stuff like right icons/actions. I guess that the the virtual list injected style doesn't play nicely with the styling from material-ui
2) Sticky subheaders get really messy as well, probably for the same reason (react-window use position absolute to layout the items)

I am going to probably try react-virtualized to see if they work better together

@oliviertassinari
Copy link
Member

@MastroLindus I assume you have tried our virtualization demo first?

@MastroLindus
Copy link

@oliviertassinari I started on my own way, found the problems I described above, googled for it, found this thread.

Then I discovered about the virtualization demo, I checked its code and found out that is basically the same thing I am doing myself (only applied to Tables instead of lists).

My issues are a bit more lists related. The general concept work, but stuff like icons, actions, sticky headers are not working properly.
In detail, referring to my 2 points in the above comment:
Issue number 1 could be an issue with react-window that is not going to be present with react-virtualized. I am going to let you know as soon as I try with it today or tomorrow. Maybe we are lucky and it will just work with react-virtualized. Let's see.
Issue number 2 (sticky subheaders not working properly) I think it's something that will need an ad-hoc solution anyway, as their positioning will probably create a conflict no matter what. Somebody built https://github.com/marchaos/react-virtualized-sticky-tree for a similar problem (having react-virtualized deal with sticky headers) but I didn't have the time to look into it right now

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 3, 2019

@MastroLindus
Copy link

MastroLindus commented Apr 3, 2019

@oliviertassinari None.
I specified that my issues are specific to virtual lists, not virtual tables. (Again: I am sorry I am writing on the issue for Table Virtualized, there's none for virtualized Lists and I was following up the comment of @mdizzy )

Regarding the lists: using ListItemSecondaryAction, ListItemIcon and ListSubheader (with sticky headers) doesn't seem to work, as their positioning gets messed up.
Without those components, I got a basic list to work with react-window (and I guess would do the same in react-virtualized)

@oliviertassinari
Copy link
Member

@MastroLindus I think that you can move the discussion to #15149.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests