shouldComponentUpdate calls for Fill and Stroke

[DesignSpaceBot]

@iwehrman Here is a really simple change for you... Immutable.is calls on the important props of Fill and Stroke. Difference is quite nice.

Then:

Now:

@mcilroyc Since you implemented these controls, you might know if what I'm doing is very stupid and would prevent us from redrawing when we need to.

-- @volfied at 2015-01-30T23:48:20Z

[DesignSpaceBot]

I'm generally strongly in favor of this, but I have a few complaints. Namely, I suspect that this implementation of shouldComponentUpdate is not quite sound because these render methods rely critically on some other properties beyond this.props.fills/strokes. In particular: this.props.readOnly, this.props.index and collection.pluck(this.props.document.layers.selected, "id"). Luckily, in this case it shouldn't hurt much to mix these into shouldComponentUpdate.

This brings me to a more general React complaint: it often seems that shouldComponentUpdate wants to do some computation that is also needed in render. For example, computing the list of IDs of the selected layers. And although shouldComponentUpdate (or componentWillMount) is always called before render, there's no structured way of getting information computed here into render besides storing it in the state, which can result in a fair amount of complexity over time. (Stateless components are great!) What I think would be ideal is if you could return some data from shouldComponentUpdate that would show up as a parameter in to render. Then you could still have a stateless component, but you also wouldn't have to do any needless re-computation.

Man, if only we could have gone to the ReactJS conference to complain about this in person!

-- @iwehrman at 2015-01-31T00:23:58Z

[DesignSpaceBot]

If we push most of those computations in the immutable models, and cache them, this problem will be just code duplication.

readOnly, selected IDs are calculated from document.layers.selected and index is the result of the map operation. Maybe also checking for props.document.layers.selected would be enough to cover the first two.

ID is used for making sure color pickers are unique, so we might need to compare that one as well.

Let's keep this review only and wait for Cory to chime in.

-- @volfied at 2015-01-31T00:36:36Z

[DesignSpaceBot]

@iwehrman Updated the SCU for Fill, checking readOnly, index and selected layers now. Our plan is to get rid of blend modes in Fill and Stroke, right?

-- @volfied at 2015-02-02T16:15:39Z

[DesignSpaceBot]

Might it save a few ticks to also implement SCU at the *List components? Just wondering if you considered that but decided against it.

-- @mcilroyc at 2015-02-02T16:54:04Z

[DesignSpaceBot]

I decided to go with the lowest component I can get to, because in FillList, we would have to also check for if any of the Fills are going to be changed.

If I'm not mistaken, shouldComponentUpdate returning false on a component will stop rendering of sub-components completely. @iwehrman I've looked at the React doc but it doesn't confirm or deny this.

-- @volfied at 2015-02-02T17:34:08Z

[DesignSpaceBot]

Yes, if shouldComponetUpdate returns false, render will not be called on the component, which prevents render from being called on child components.

-- @iwehrman at 2015-02-02T17:40:51Z

[DesignSpaceBot]

But that's what we want, right? If the List SCU returns false then it won't need to loop over the individual items and call SCU on each. The component won't Go Away it will just not Re-Render. right?

-- @mcilroyc at 2015-02-02T17:43:42Z

[DesignSpaceBot]

Yes. And, yes, this is what we want.

-- @iwehrman at 2015-02-02T17:47:42Z

[DesignSpaceBot]

This looks good, but I should have mentioned that Stroke has the same issue. If you make the same change for Stroke I'll push the big green button :white_check_mark:.

-- @iwehrman at 2015-02-02T20:17:55Z

[DesignSpaceBot]

@iwehrman Rebased and good to go!

-- @volfied at 2015-02-03T16:50:20Z

[DesignSpaceBot]

I don't see much gains in putting a SCU in Stroke/Fill list, individual Stroke and Fill components were the slower and more time wasted areas.

-- @volfied at 2015-02-03T16:51:16Z

[DesignSpaceBot]

LGTM!

-- @iwehrman at 2015-02-03T19:12:24Z