allen-cell-animated / website-3d-cell-viewer

Other
4 stars 4 forks source link

Fix sliders not updating during playback #267

Closed frasercl closed 1 month ago

frasercl commented 1 month ago

Review time: tiny (<5min)

Resolves #264: sliders update correctly on playback again.

SmarterSlider is a utility component which wraps the (old) React wrapper around nouislider that we use, preventing it from updating while the slider is sliding. This was done to fix a performance issue (which I was unable to replicate?).

In #256, I converted SmarterSlider to a function component, which meant moving from the class component way to prevent updates (shouldComponentUpdate) to the function component one (React.memo). But I missed when I did this that the function passed to React.memo has the opposite expectation of shouldComponentUpdate: it should return true when the component should skip rendering. So I flipped some booleans around and renamed a couple variables and this component seems to be working as normal again.

toloudis commented 1 month ago

Also, does this have implications on update while dragging the slider? Because currently I think that behavior is correct, before this fix

frasercl commented 1 month ago

Also, does this have implications on update while dragging the slider? Because currently I think that behavior is correct, before this fix

Event handlers that update state will still run and the handle of the slider will still move correctly. That's all handled in nouislider, not nouislider-react. What this is intended to prevent is rerenders of the nouislider-react component when the value of the slider changes in props. Those renders basically just imperatively change the position of the slider handle again, even though it was already changed by nouislider itself in response to the user's dragging.