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

Option to limit the number of rows/columns CellMeasurer measures #962

Closed
tlrobinson opened this issue Jan 10, 2018 · 5 comments
Closed

Option to limit the number of rows/columns CellMeasurer measures #962

tlrobinson opened this issue Jan 10, 2018 · 5 comments

Comments

@tlrobinson
Copy link

tlrobinson commented Jan 10, 2018

I currently have some custom code to measure column sizes based on a limited number of rows (currently 10 non-empty cells per column). This is a reasonable heuristic in our application: it's ok if some cells overflow.

I've tried porting it to use CellMeasurer instead, and it works ok except there are too many rows to measure all of them and have decent performance.

Is there a way I could limit the number of rows used in measuring, or better yet, pick specific rows for each column to measure (i.e. non-empty ones)? If not, is this a feature you'd consider adding (I may be able to help)?

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

This is related to the performance considerations section of the docs. If we only measured some of the rows (for example) then as a user scrolled, it would be possible for newly encountered rows to have wider columns, requiring us to readjust column sizes, and producing a weird visual effect of columns shifting around as a user scrolls. This would be very disorienting. So long story short: The current functionality is there for a reason, even though I understand it has perf impacts.

That being said, CellMeasurerCache is not that big so you could always fork it and implement the behavior you seek there. If after doing this, you think you've got something that's generically useful for other users, a PR would be cool to talk it over. 😄

@bvaughn bvaughn closed this as completed Jan 13, 2018
@tlrobinson
Copy link
Author

tlrobinson commented Jan 15, 2018

@bvaughn Thanks for the quick response

I understand with the default behavior if we only measured some columns initially then more later it could result in the column/row size shifting, but in my case I want to measure a subset of cells then lock the row/column size, even if it means some cells will overflow. This type of "probabilistic" sizing is fine in my application, and much preferable to the performance implications of measuring thousands of cells up front.

I poked around at CellMeasurerCache and don't quite understand how to modify it to prevent Grid from measuring every cell. It looks like if deferredMeasurementCache is passed to Grid then batchAllCells value of true is passed to ScalingCellSizeAndPositionManager (for the non-fixed direction) which causes getVisibleCellRange to return the full range.

Perhaps what I want is a custom cellRangeRenderer that only renders certain cells the first render? Or maybe a custom cellCache?

@bvaughn
Copy link
Owner

bvaughn commented Jan 15, 2018

Your use-case sounds pretty specific to me though. I'd be happy to review a PR (if you can implement it in a way that would also benefit others) but I don't think I'm interested in building that functionality myself. 😄

My original thinking with CellMeasurerCache was that you might be able to accomplish what you want by forking it just adding logic in so that any unmeasured column in a row with some threshold of measurements already taken just returns a width as though it has already been measured. (Ditto for the height of a row in a given column with > N measurements.)

That being said, my intuition was incorrect. Looking at CellSizeAndPositionManager, I see that it will render all of the columns (or rows, depending) even if they have already been measured. This isn't great. Ideally, Grid should only pay that cost for ranges that intersect with unmeasured cells. Hm!

I wonder if something like this branch would solve the problem? Need to think about this. I may be overlooking something obvious.

Edit: (For clarity.) My PR addresses an unnecessary performance cost when using CellMeasurer and Grid but does not directly address your request. I'd still suggest you implement that yourself and (if you're willing) share it back via a PR.

@bvaughn
Copy link
Owner

bvaughn commented Jan 15, 2018

I've pushed my branch to #969.

@berkowitze
Copy link

I'm running into this "issue" right now (I realize it's not really an issue and is just a side effect of using dynamic widths, and also this is a 3 year old thread). Two possible solutions for anyone in a similar position though:

  1. If the performance issues are more due to cells being expensive to render than to sheer number of cells, consider short-circuiting the rendering of the out-of-view cells (using isVisible) with simple divs with set guessed widths. Once isVisible is true, the proper cell is rendered and you can use CellMeasurer's provided measure function to update the column width (leading to potentially increasing column widths on scroll, but that might not be an issue for some people depending on typical column contents).

  2. It might be possible to hack this request by using the keyMapper input to CellMeasurerCache, modding the row index by the number of rows you want measured. You'd have some over-length content in that case, but you can ellipsize or handle that however works best. Haven't tried this though. I'd imagine it has negative implications for filtering/sorting if those are parts of the application.

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

No branches or pull requests

3 participants