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

Version 8 Roadmap #386

Closed
4 of 7 tasks
bvaughn opened this issue Sep 14, 2016 · 30 comments
Closed
4 of 7 tasks

Version 8 Roadmap #386

bvaughn opened this issue Sep 14, 2016 · 30 comments

Comments

@bvaughn
Copy link
Owner

bvaughn commented Sep 14, 2016

I have begun working on react-virtualized version 8 (see PR #387). This issue outlines my intended changes. I welcome input from the community- so please feel free to comment below if you think certain features should (or should not) be included in the next upcoming major release.

Upgrade instructions and jscodeshift mods are available here.

High level goals

  • Improve performance, particularly scrolling performance.
  • Reduce learning curve; make things simpler for first-time users!

Progress

Planned changes:

  • Rename components (FlexColumn, FlexTable, VirtualScroll)
  • Remove .Grid_cell wrapper <div>
  • CSS class names
  • All functional styles moved inline

Potential changes:

  • More consistent index params
  • Measure CellMeasurer content within parent component
  • Support measurement persistence

Planned changes

Rename components

Certain components in react-virtualized have arbitrary names. For example, VirtualScroll is ambiguous and FlexTable is unnecessarily specific. In order to improve the first-time-user experience, version 8 will rename a few components.

Version <= 7 Version 8+
FlexColumn Column
FlexTable Table
VirtualScroll List

A codemod is available for this change:

jscodeshift -t /path/to/react-virtualized/codemods/7-to-8/rename-components.js source

Remove .Grid_cell wrapper <div>

My single biggest focus with recently updates to this library has been improving scrolling performance. Removing unnecessary DOM elements is a win. To this end, version 8 will be removing the cell wrapper and passing style information directly to your cellRenderer to be used. In other words this:

<div class="Grid__cell" style="left; top; height; width;">
  <div class="your-custom-class" style="your-custom-style;">
    <!-- Your content -->
  </div>
</div>

...will become this:

<div class="your-custom-class" style="position; left; top; height; width; your-custom-style;">
  <!-- Your content -->
</div>

This will also enable me to remove the following style-related properties (as you will have full control over the wrapper cell yourself):

Component Properties to be removed
Grid cellClassName, cellStyle
VirtualScroll rowClassName, rowStyle
FlexTable rowWrapperClassName, rowWrapperStyle

This change also means that your cellRenderer will be responsible for a few more things. Below is an example of a version 8 cellRenderer. Note that key and style parameters are new:

function cellRenderer ({ 
  columnIndex, // Horizontal (column) index of cell
  isScrolling, // The Grid is currently being scrolled
  key,         // Unique key within array of cells
  rowIndex,    // Vertical (row) index of cell
  style        // Style object to be applied to cell
}) {
  // Grid data is a 2d array in this example...
  const user = list[rowIndex][columnIndex]

  // If cell content is complex, consider rendering a lighter-weight placeholder while scrolling.
  const content = isScrolling
    ? '...'
    : <User user={user} />

  // It is important to attach the style specified as it controls the cell's position.
  // You can add additional class names or style properties as you would like.
  // Key is also required by React to more efficiently manage the array of cells.
  return (
    <div
      key={key}
      style={style}
    >
      {content}
    </div>
  )
}

There is no codemod for this change due to its complexity.
However you may use the following HOF to maintain backwards compatibility and allow you to migrate cell renderers one at a time.

// Can be used for Grid, List, or Table
function createCellRenderer (cellRenderer) {
  return function cellRendererWrapper ({ key, style, ...rest }) {
    return (
      <div
        key={key}
        style={style}
      >
        {cellRenderer(rest)}
      </div>
    )
  }
}

// Demonstrates example usage
function renderGrid (props) {
  const { cellRenderer, ...rest } = props

  return (
    <Grid
      cellRenderer={createCellRenderer(cellRenderer)}
      {...rest}
    />
  )
}

CSS class names

As of this time there is still no single way the community has agreed on regarding styles within React. This is particularly tricky for a library as styling choices can impose constraints on application code. For example if a library component chooses to use inline styles then an application is required to also use inline styles (or the !important directive) in order to override defaults. Because of this, react-virtualized will continue to use global/static class names for the time being.

However generic class names like .Grid (or .List, .Table, etc) can also cause complications for application code. To address this, version 8 class names will use a .ReactVirtualized__ prefix. For example, the outer-most class for a Grid component will be ReactVirtualized__Grid.

All functional styles moved inline

Collection, Grid, and List used to depend on an external react-virtualized styles.css file for their static styles. With version 8 this is no longer the case. All functional styles (eg position, overflow, etc) have been converted to inline styles. This means that for many apps- the external stylesheet will no longer be required at all.

Note that the Table component also includes several presentational styles by default. These will remain in the styles.css file to enable them to be easier for application code to override.

Potential changes

More consistent index params

Grid renderers receive 2 properties: columnIndex and rowIndex while corresponding VirtualScroll and FlexTable methods receive only a single parameter, index. This is fairly intuitive but it increases the difficulty of creating HOCs that are capable of working with etiher type of component because it requires users to rename/map parameters (eg one({ index }) => two({ columnIndex: 0, rowIndex: index })).

As of version 8, all 3 components (Grid, List, and Table) will use the more qualified names. Table will pass a rowIndex parameter to its children and List will pass rowIndex.

Measure CellMeasurer content within parent component

CellMeasurer renders its content to a hidden div mounted at the root of the document. This has a couple of drawbacks:

  • Rendered content can't be moved from the document to the (eventual) intended parent and so it must be thrown away and recreated (which is expensive).
  • Inherited styles don't get applied during the first render which can cause the measurements to be inaccurate.

Currently neither Grid nor CellMeasurer are aware of each other and so this cannot be avoided. However it should be possible for Grid (or List or Table) to provide a way for CellMeasurer to render content inside of itself so as to re-use the rendered content for display purposes after it has been measured.

It is possible that this ends up adding more complication than it is worth and this feature gets dropped. It is also possible that I will be able to implement this feature in a way that does not impact the external API.

Support measurement persistence

Mentioned by @jquense below.

@edulan
Copy link
Contributor

edulan commented Sep 14, 2016

Cool @bvaughn! I really like the version 8 roadmap 👏

Regarding components naming I have some concerns though. You're planning to rename VirtualScroll to List and I like the idea, but I'm wondering if this may create some confusion to RV users. List name is so common that I'll be a bit hard to differentiate between an app's component or RV one. Obviously you can infer it taking a look at the props the component receives, but surely it'll take more time that just looking at the name.

I was thinking that maybe using the prefix Virtual for all components could be interesting. So this is how it'd look like:

Grid => VirtualGrid
FlexTable => VirtualTable
VirtualScroll => VirtualList
Collection => VirtualCollection

Though it's possible to alias all these components when importing, I think this naming it's more self explanatory. What do you think?

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 14, 2016

Interesting feedback @edulan. Firstly, thanks for taking the time to share your thoughts! 🙇

I think you raise a valid concern. I'll admit I'm a bit averse to lengthier names. In particular "virtual" as it's a bit awkward to type (even for me- and I type it all the time). I think even with my initial proposed names there are a couple of easy workarounds for the ambiguity you mention:

import { Grid as VirtualGrid } from 'react-virtualized';
// <VirtualGrid/>

import Virtual from 'react-virtualized';
// <Virtual.Grid/>

I also think there's some potential awkwardness in terms of when the "virtual" prefix would stop being used. For example, does a VirtualTable contain Columns or VirtualColumns? I assume the latter for symmetry, but there's nothing virtual about the columns. Or at least they aren't doing any windowing so it feels a bit odd to prefix them with that name. Similar for a component like CellMeasurer. Is it now VirtualCellMeasurer? Its name was already reasonable unique. etc.

Let me sit on this a bit (and also give others a chance to weigh in). I'm on the fence about it. 😁 Great feedback though!

@jquense
Copy link
Contributor

jquense commented Sep 14, 2016

one thing I'd like that been tough for us is the ability to persist cached measurements across list mounts and unmounts of a list. An example: when navigating back to a page we restore scroll position for the list, which triggers cell measures for each row up to the needed scroll position, since this is still rendering each row it's essentially just as expensive as naively displaying the whole list up till that point. it'd be nice if we could save and use the measurements from the last mount since it's likely to be the same for subsequent mounts (at least for a short time period anyway).

I may also be misunderstanding hie it interplay with estimated row height but that seems to be what's happening, tho I may be speaking to soon. In any case being able to control that state would still be useful

@edulan
Copy link
Contributor

edulan commented Sep 14, 2016

@bvaughn I was thinking mainly in container elements (Grid, Collection, FlexTable and VirtualScroll). The other HOC component names are fine and as you said are not related with virtualization at all.

As for VirtualTable you could take the same approach used in React-Bootstrap modals and declare subcomponents this way:

<VirtualTable>
  <VirtualTable.Column>
    // ...
  </VirtualTable.Column>
</VirtualTable>

Either way you know, we're facing one of the two hardest things in CS 😆

@oyeanuj
Copy link
Contributor

oyeanuj commented Sep 14, 2016

@bvaughn This looks great. My only addition would be something I was trying to do with RV but was not possible, and yet to me, seems like a really good case for RV.

A cleaner solution for infinite loading list of feed of elements with variable heights where the initial heights may not be known.

Feel free to let me know if it seems like a one-off case or not on the priority of use-cases : Maybe this is a docs thing, but at some point, it didn't seem to work.

Thanks for soliciting feedback from the community though :)

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 14, 2016

@bvaughn I was thinking mainly in container elements (Grid, Collection, FlexTable and VirtualScroll). The other HOC component names are fine and as you said are not related with virtualization at all.

That's fair. Although I think that since they're all already namespaced within the Virtual import (or whatever you name it) it's a bit redundant to tack another "virtual" prefix on.

@jquense That's an interesting use-case that I hadn't considered- measurement persistence. I'll add it to the list of potentials above.

Thanks @oyeanuj! It sounds like what you're describing should already be possible combining InfiniteLoader + CellMeasurer + VirtualScroll. Have you tried that? Let's chat on Gitter or something about it.

@iyn
Copy link

iyn commented Sep 15, 2016

I appreciate the work so far! |Could you give a rough estimate of when v8 will be in alpha/beta? Absolutely no pressure, I'm simply curious whether it's closer to say 1 month or 3-4 months/longer.

Thanks!

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 15, 2016

@iyn No problem! That's a reasonable question! I would expect an RC within a week or two. Depends on my availability as I'm currently traveling in China- but general I move pretty quickly with changes like this to avoid having to maintain 2 versions.

PS Could go faster if anyone would like to chip in and help write code mods. Let me know if you're interested!

@iyn
Copy link

iyn commented Sep 15, 2016

@bvaughn Thanks for the swift response :). This is great to hear. I'm currently migrating my custom data table module to use react-virtualized, I'll probably use v7 API for now, but given your estimate I won't have a lot to change when I'll move to v8 :).

Again - big thank you for your work and such positive attitude!

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 15, 2016

No problem! I think version 7 to 8 should not be too complicated a migration path. Mostly a few renamed components or properties. Biggest challenge will be the cell renderer change and I've got a little HOC that should make that easy to do iteratively.

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 17, 2016

FYI, release candidate 1 has been published to NPM as react-virtualized@8.0.0-rc.1. All of the "planned changes" have been made. I've decided for the time being to skip the index parameter rename and to defer the cell measurement persistence. (I think both should be possible in a non-backwards-breaking way, if I do them.)

@coluccini
Copy link
Contributor

What about InfiniteLoader being compatible with Collection?

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 18, 2016

@coluccini Collection data is non-linear (meaning an item at index X may appear after an item at index X+1). This is the primary thing that differentiates Collection and Grid. It also makes Collection incompatible with InfiniteLoader since InfiniteLoader is built on the premise of linear data.

It's possible you have a specific use case for Collection where this makes sense but I don't think it makes sense in most cases given those constraints. I'd suggest you consider forking InfiniteLoader for your purposes. See if you can get it working with Collection. Then maybe share your findings with me and if it looks like something that could be used by more than just your use case- maybe we can add support to react-virtualized.

@coluccini
Copy link
Contributor

I understand that's Collection data can be non-linear. But I think the most common use of Collection it's to make pinterest-like layouts. And that's linear data, just ordered and wrapped horizontally, don't you think?

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 18, 2016

That may happen to be a common use case of Collection but since it's not a requirement of the component, I can't add constraints that would make it one. Make sense?

@nhducit
Copy link

nhducit commented Sep 18, 2016

I think it will be great if we have these features:

  • resize column
  • drag-drop column to reorder
  • support both vertical and horizontal scroll
  • freeze n column
  • First column as header

example: https://www.ag-grid.com/example.php

@coluccini
Copy link
Contributor

coluccini commented Sep 18, 2016

That may happen to be a common use case of Collection but since it's not a requirement of the component, I can't add constraints that would make it one. Make sense?

Yes, it does 😊

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 18, 2016

@nhducit

  • resize column

I recommend using react-draggable for this. Easy to use and works well with react-virtualized. I've used this combination myself in production apps it works great. 😄

  • drag-drop column to reorder

This is a windowing library. Drag and drop is its own, very large space. However there is a fantastic library by @clauderic called react-sortable-hoc that can be used with react-virtualized for drag-and-drop behavior. I've used this myself in production apps and have been very happy with the results. 😁

  • support both vertical and horizontal scroll

This has been supported ever since version 1 by the Grid component.

  • freeze n column
  • First column as header

Sticky columns and rows are possible already using the ScrollSync HOC. See a demo here.

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 18, 2016

Version 8 is live!

Check out the upgrade steps for instructions on how to migrate from version 7. Let me know if you encounter any issues!

@bvaughn bvaughn closed this as completed Sep 18, 2016
@IanVS
Copy link
Contributor

IanVS commented Sep 20, 2016

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 20, 2016

Aye, I'll correct it. Thanks. :)

@IanVS
Copy link
Contributor

IanVS commented Sep 20, 2016

These are some great changes, bravo!

As of version 8, all 3 components (Grid, List, and Table) will use the more qualified names. Table will pass a rowIndex parameter to its children and List will pass rowIndex.

From looking at the docs* and the code it seems that ^ was not implemented for List, which still provides index, is that correct? If so, are there still plans to make this change?

*From the List docs:

rowRenderer : Responsible for rendering a row given an index. Signature should look like ({ index: number, isScrolling: boolean }): React.PropTypes.node

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 20, 2016

Thanks @IanVS!

That' correct. That was one of the potential changes but it didn't make the cut. (Only the ones checked off up top made the cut.)

I was really on the fence about the index thing. I implemented it in a branch but decided against moving forward with it because- once I saw it- it felt kind of awkward for vertical-only scrolling.

@IanVS
Copy link
Contributor

IanVS commented Sep 20, 2016

Cool, thanks for explaining. I think it accidentally snuck into some of the docs ;)

#402

@bvaughn
Copy link
Owner Author

bvaughn commented Sep 20, 2016

Yeah. Whoops. 😄 It was a pretty big change-set and I was pretty jet lagged from returning from China so... I struggled a bit 😆

niik added a commit to desktop/desktop that referenced this issue Oct 6, 2016
See bvaughn/react-virtualized#386

This gives us one level of nesting less in our lists
@jedwards1211
Copy link
Contributor

@bvaughn are true sticky rows possible though, where there can be multiple header rows throughout the data rows and the topmost one will stick to the top of the scroll container? If not I'm going to see if it's possible to use react-sticky with react-virtualized.

@bvaughn
Copy link
Owner Author

bvaughn commented Oct 13, 2016

This is an example of what I mean by sticky rows and columns:
https://bvaughn.github.io/react-virtualized/#/components/ScrollSync

The top-most row and left-most column stick. (It could just as easily be the top-several rows, or a random column, (although that would require an additional Grid). Is this what you're referring to?

Never heard of react-sticky. Would be interested in seeing what you come up with, if you combine it with react-virtualized. 😄

@IanVS
Copy link
Contributor

IanVS commented Oct 13, 2016

I believe what @jedwards1211 is looking for is more like this: https://www.youtube.com/watch?v=EJm7subFbQI&feature=youtu.be

It's something I have attempted to do, but ended up falling back to a separate component which lives on top of the scroll container and just updates when a header row reaches the top. If you find a way to achieve what you want, @jedwards1211, I'd love to hear how you do it!

@jedwards1211
Copy link
Contributor

@bvaughn right, I'm looking for what @IanVS suggested, which seems to be the accepted meaning of "sticky", just as in the upcoming CSS position: sticky. One thing I realize will be a problem though is the sticky rows would still need to render when scrolled past the top of the List.

@bvaughn
Copy link
Owner Author

bvaughn commented Oct 13, 2016

I see! I think this could be done with react-virtualized but it would require some custom logic. I believe it would be really cool to get that custom logic wrapped up in a HOC that could be shared with others though. If either of you feel like sharing back your solutions for this that is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants