gdotdesign / elm-ui

UI library for making web applications with Elm
https://elm-ui.netlify.com
BSD 2-Clause "Simplified" License
920 stars 39 forks source link

Ui.Chooser.view doesn't do a true lazy render #51

Closed mrozbarry closed 7 years ago

mrozbarry commented 7 years ago

I've run into a bit of a hitch related to my previous issue ( #50 ). So I have on average, 100 drop downs where each drop down selection will cause data changes in the other drop downs. Your code here will force a redraw of all options, even though most of my choosers are not opened (I have closeOnSelect).

I was able to modify your code on the line I linked to:

children =
  if model.open then
    (List.map (renderItem model) (items model))
  else
    []

The performance boost was huge for my use case. Even doing a render on open with 500 items, it wasn't laggy or visibly offensive. Having 1000 was laggy, so maybe there is a better optimization here.

rapind commented 7 years ago

Could potentially pass a renderWhenClosed boolean for edge cases where you have really long lists and would want to render the children ahead of time?

gdotdesign commented 7 years ago

I need to do some experiments, in general I agree that this is an issue, having this in would be a quick fix. My concern here is that there would be a small time during closing when the drop-down is visible but would not have content.

Maybe rendering the items lazily https://github.com/gdotdesign/elm-ui/blob/master/source/Ui/Chooser.elm#L482 would be enough. If you could try that that would help a lot since you have a setup already: (List.map (Html.Lazy.lazy renderItem model) (items model))

@rapind yeah I think that can be a good solution as well, having default to True

mrozbarry commented 7 years ago

@gdotdesign I'm already using Ui.Chooser.view to use Html.Lazy, but since I'm changing potentially >= 50 choosers, each with potentially >= 50 items (which will be updated each time) each, the elm virtual dom implementation just chokes trying to get everything updated, resulting in a very noticeable delay when I select an item (which is when I'm able to filter the lists in all the other choosers). If I had a set of static choosers, then I think Ui.Chooser.view would be incredibly performant, but since my items in each chooser will react to changes from any other chooser, it is not working as expected.

From my own experiment, before the code change I suggested, clicking on an item would take >500ms to process. After implementing that conditional child render, it went to <100ms.

To reproduce the sort of code I'm dealing with, I have a table of four columns, 20 rows, and each cell has a chooser. Each chooser has potentially 100 items. Each time an item is chosen, it cannot be chosen in any other select, and to achieve this, we filter out the value of the selected chooser in all other choosers. I'll see if I have some time this weekend to give you a proper small project to demonstrate this, just so we're all looking at the same performance issue.

rapind commented 7 years ago

PR https://github.com/gdotdesign/elm-ui/pull/52

gdotdesign commented 7 years ago

With this commit: https://github.com/gdotdesign/elm-ui/commit/8326eb7c44353dc852102da5a6c9965a6f65cfbf I've added the renderWhenClosed field which decides whether or not to render items (defaults to True).

I also added a test application as @mrozbarry suggested: 100 choosers with 100 items each updating the other choosers values when any changes (running it with elm-reactor is really not performant because it updates the debug view, to test it compile it with elm-make: elm-make spec/Ui/ManyChooserSpec.elm).

This will be in the next release.

gdotdesign commented 7 years ago

Released with 1.1.0 https://github.com/gdotdesign/elm-ui/releases/tag/1.1.0

rapind commented 7 years ago

This looks great @gdotdesign !