GriddleGriddle / Griddle

Simple Grid Component written in React
http://griddlegriddle.github.io/Griddle/
MIT License
2.5k stars 378 forks source link

Using customComponent: the component unmounts and re-mounts in every Griddle re-render #750

Closed rdavidwu closed 7 years ago

rdavidwu commented 7 years ago

Hi, I have a table with two columns that are using custom components. One of them has a progress bar, whose intended purpose is to show the progress of an upload. When a file is being uploaded, Griddle re-renders accordingly every time there is a new progress and the progress bar updates. Now, I have a second column with a second custom component that is supposed to cancel the upload when an element inside it is clicked. However, this click is not being registered consistently because the component keeps mounting and unmounting at a rapid rate (because the upload progress updates quickly). I verified this by adding logs in the componentDidMount() method. Is there a way to make Griddle not remount the cell or row components for every re-render?

dahlbyk commented 7 years ago

There are almost certainly opportunities to reduce Griddle rerenders, but my first instinct is that you're going to have better luck keeping progress state internal to that component.

joellanciaux commented 7 years ago

Agreeing with @dahlbyk, I think what would be required in the case that you're mentioning would be some extra data diffing to discern which values in which rows are updating, which we're not really doing at the moment.

Something that might allow you to bypass updates immediately would be something similar to:

const progressCell = connect((state, { value }) =>
  // Reach out to your store to grab the current progress.
  return {
    progress: getProgressForItem(state, value),
  };
)(({ progress }) => {
  // Fancy progress bar here! 
  return (
    <span>{progress}</span>
  );
});

// In your ColumnDefinition...
<ColumnDefinition ... customComponent={progressCell} />

Extra diffing between data updates could be a cool opt-in feature if we do something like:

<Griddle ... mutable />

to include some additional shouldComponentUpdate functionality (needs a better name). Thoughts @dahlbyk @ryanlanciaux?

dahlbyk commented 7 years ago

I have no idea what it would look like, but better lifecycle hooks couldn't hurt.

That said, mutating a full data set will almost certainly be harder to reason about than keeping mutation compartmentalized.

joellanciaux commented 7 years ago

After diving in a little bit, I don't a specific mutable flag or anything of the like would be necessary. It looks like there are only a handful of things that needed to change for this behavior to be better supported.

This behavior should be A-OK with #755