Delaying update of height and width in Layout #3180

wuweiweiwu · 2018-03-10T00:01:29Z

Issue:

What I did

fixes #1990
Delayed the setState call so we don't rerender a billion times.

Plus removed adding un-needed event handlers to window

How to test

use React inspector to check state updates

yarn start

resize the windows and see that the previewPanelDimensions state isn't updated thus doesn't rerender until after you stopped dragging for a little bit (50ms)

Is this testable with jest or storyshots?

No

Does this need a new example in the kitchen sink apps?

No

Does this need an update to the documentation?

No

codecov · 2018-03-10T00:11:06Z

Codecov Report

Merging #3180 into master will decrease coverage by 0.04%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
- Coverage   36.02%   35.98%   -0.05%     
==========================================
  Files         437      437              
  Lines        9465     9476      +11     
  Branches      903      905       +2     
==========================================
  Hits         3410     3410              
- Misses       5460     5470      +10     
- Partials      595      596       +1

Impacted Files	Coverage Δ
lib/ui/src/modules/ui/components/layout/index.js	`27.15% <16.66%> (-2.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7daa522...31e2a2b. Read the comment docs.

danielduan · 2018-03-10T01:56:04Z

lib/ui/src/modules/ui/components/layout/index.js

@@ -140,14 +140,6 @@ class Layout extends React.Component {
    this.onDragEnd = this.onDragEnd.bind(this);
  }

-  componentDidMount() {
-    window.addEventListener('resize', this.onResize);


are these not needed anymore?

I think we still need to handle the case when the browser window gets resized?

I don’t think so. Because onResize returns another function the window resize events never actually updates the state.

I will have to double check but I think the SplitPane onChange handler fires when browser window is resizes.

Yes we need. Try to resize window here https://deploy-preview-3180--storybooks-official.netlify.com

@Hypnosphi I also doubled checked on master resizing the browser window never triggered onResize

I'm not sure how we should handle browser window resizing since the Layout onChange handlers don't fire

I also doubled checked on master resizing the browser window never triggered onResize

Oh I see. Looks like a bug worth fixing to me. Probably caused by onResize returning a function

We should probably extract the part that updates previewPanelDimensions from onResize, because it's the only thing that should be done on window resize

Sounds good!. I'll change that in this PR!

Hypnosphi · 2018-03-10T07:09:27Z

lib/ui/src/modules/ui/components/layout/index.js

+            height: clientHeight,
+          },
+        });
+      }, 50);


Please use lodash.debounce package

Or maybe it should rather be lodash.throttle, see #3180 (comment)

@Hypnosphi Sounds good. I discovered a new issue with using _.throttle the way that it is currently implemented. it will create a new throttled function on every state update which is bad. Thus I will be changing how the onResize function and how its passed in to the onChange handlers

Just apply it in constructor

this.onResize = throttle(this.onResize.bind(this), 200);

I tend to use that method of writing bound class methods a lot.

for readability purposes, it might be better to name it this.onThrottledResize = ... to differentiate between the two.

Hypnosphi · 2018-03-10T07:21:14Z

I tried it and I can't say I like it:

https://deploy-preview-3180--storybooks-official.netlify.com/?selectedKind=ui%2FAddonPanel&selectedStory=default&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Fstories%2Fstories-panel

Dimensions panel is used for instant feedback, so picking debouncing over throttling doesn't look like a good choice to me

ndelangen · 2018-03-10T13:30:28Z

Can we debounce on requestAnimationFrame?

ndelangen · 2018-03-10T13:46:41Z

@wuweiweiw thank you for helping, really appreciate the efforts! I think the immediate feedback of the size is really important, if someone is targetting 800px wide for example, it's annoying to not yet fast feedback, and likely 'overshoot' multiple times.. Do you agree?

I think if we can throttle on requestAnimationFrame everything should be optimal right?

danielduan · 2018-03-10T17:06:37Z

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint.
https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

From my understanding of these docs, it seems like this should be used when animating or whenever there's a constraint on browser rendering speed. In this case, I believe the setState updates are a constraint on CPU cycles so I'm not sure if this is an appropriate use.

I'm okay with either throttle or requestAnimationFrame. At this point, it seems like a micro optimization.

Hypnosphi · 2018-03-10T19:09:34Z

Can we debounce on requestAnimationFrame?

It's only about updating numbers on dimension panel, no need for 60 fps animations

wuweiweiwu · 2018-03-10T21:30:18Z

@Hypnosphi @danielduan @ndelangen done!

I rewrote the onResize function so it doesn't return a function. And thus a new function wouldn't be created in render() every time. The _.throttle is set to 200ms but that can be changed :)

Hypnosphi · 2018-03-10T21:41:15Z

lib/ui/src/modules/ui/components/layout/index.js

 import PropTypes from 'prop-types';
 import React from 'react';
 import SplitPane from 'react-split-pane';
+import _ from 'lodash'; /* eslint-disable-line import/no-extraneous-dependencies */


We use per-function lodash packages. You need to add lodash.throttle as a dependency to package.json in lib/ui, and import it like that:

import throttle from 'lodash.throttle';

By the way, that eslint warning you've suppressed was there for a reason. If you use a package that we don't have as a direct or peer dependency, it will break on user side, because they might not have this dependency

Is it? I still see this import and eslint-disable

Oh shoot. I think I accidentally dropped that commit when I was rebasing

Hypnosphi · 2018-03-10T21:50:03Z

And thus a new function wouldn't be created in render() every time

Actually, it is created every time, now it's just created directly in render (the arrow function)

wuweiweiwu · 2018-03-10T21:55:11Z

@Hypnosphi My mistake. But I just got rid of the way onResize was returning the function that took in size and moved it inside render. I think that makes more sense

And if we put throttle on the inner function returned. It would be executed every time onSize was called

Hypnosphi · 2018-03-10T22:13:52Z

The way it was before is more functional-style, the way you do it is more imperative-style. I have no strong feelings about it though

wuweiweiwu · 2018-03-11T12:20:04Z

@Hypnosphi I also added the throttled event handler to window resize events.

Is there anything else I should do in this PR?

Hypnosphi · 2018-03-11T18:41:32Z

lib/ui/src/modules/ui/components/layout/index.js

-  onResize(pane, mode) {
-    return size => {
+  onResize(pane, mode, size) {
+    if (pane && mode && size) {


Looks a bit hacky to me. Let's extract the part that updates state into a separate function instead.

In fact, the state updates should be throttled (because they provide visual feedback), and the saveSizes part should be debounced (because it only makes sense to store sizes when they are final)

Sounds good!

@Hypnosphi seems that size is used to get defaultSize and that value is only changed when you save it to local storage. Thus I was getting some weird behavior when I was debouncing. So I opted to throttle both.

I also made saveSize on a smaller interval for smoother user experience

wuweiweiwu · 2018-03-11T21:48:14Z

@Hypnosphi Fixed! I think we can even get rid of the throttling of saveSize but I'm not sure how expensive it is to save to localStorage

wuweiweiwu · 2018-03-11T21:52:06Z

And also it seems we can call getSizes in componentDidMount and save the current size in local state. And then we can use lodash.debounce to update the size in localStorage
Thus we would only access localStorage when we actually need to instead of fetching on every render.

But I think that is out of scope for this PR and I can do it in another :)

Hypnosphi · 2018-03-11T22:37:57Z

Cool, works like a charm now!

wuweiweiwu force-pushed the issue-1990 branch 2 times, most recently from cebb1d2 to 48c95cd Compare March 10, 2018 00:11

wuweiweiwu force-pushed the issue-1990 branch from 48c95cd to 2038bfe Compare March 10, 2018 00:13

danielduan reviewed Mar 10, 2018

View reviewed changes

Hypnosphi requested changes Mar 10, 2018

View reviewed changes

Hypnosphi added ui performance issue labels Mar 10, 2018

wuweiweiwu force-pushed the issue-1990 branch from 8c3514e to cb6ed00 Compare March 10, 2018 21:26

Hypnosphi requested changes Mar 10, 2018

View reviewed changes

wuweiweiwu force-pushed the issue-1990 branch 2 times, most recently from 0b27b6f to 65329d3 Compare March 11, 2018 12:05

Hypnosphi reviewed Mar 11, 2018

View reviewed changes

wuweiweiwu force-pushed the issue-1990 branch from f515154 to ab70d3c Compare March 11, 2018 21:17

wuweiweiwu added 4 commits March 11, 2018 14:18

delaying update of height and width until after stop dragging 50ms

b0c7462

rewriting onResize so it doens't return a function. Throttled to 200ms.

aaed492

reusing onThrottledResize on window resize events

ab70d3c

throttling saveSize and updateState. Separating logic

31e2a2b

Hypnosphi approved these changes Mar 11, 2018

View reviewed changes

Hypnosphi merged commit 123a9c0 into storybookjs:master Mar 11, 2018

dependencies bot mentioned this pull request Jan 15, 2021

Update 3 dependencies from npm #2257

Merged

Delaying update of height and width in Layout #3180

Delaying update of height and width in Layout #3180

Conversation

wuweiweiwu commented Mar 10, 2018 • edited Loading

What I did

How to test

Is this testable with jest or storyshots?

Does this need a new example in the kitchen sink apps?

Does this need an update to the documentation?

codecov bot commented Mar 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Hypnosphi commented Mar 10, 2018

ndelangen commented Mar 10, 2018

ndelangen commented Mar 10, 2018

danielduan commented Mar 10, 2018

Hypnosphi commented Mar 10, 2018

wuweiweiwu commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Mar 10, 2018 • edited Loading

wuweiweiwu commented Mar 10, 2018 • edited Loading

Hypnosphi commented Mar 10, 2018

wuweiweiwu commented Mar 11, 2018

Hypnosphi Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu commented Mar 11, 2018

wuweiweiwu commented Mar 11, 2018

Hypnosphi commented Mar 11, 2018

wuweiweiwu commented Mar 10, 2018 •

edited

Loading

codecov bot commented Mar 10, 2018 •

edited

Loading

wuweiweiwu Mar 10, 2018 •

edited

Loading

Hypnosphi Mar 10, 2018 •

edited

Loading

Hypnosphi Mar 10, 2018 •

edited

Loading

danielduan Mar 10, 2018 •

edited

Loading

Hypnosphi commented Mar 10, 2018 •

edited

Loading

wuweiweiwu commented Mar 10, 2018 •

edited

Loading

Hypnosphi Mar 11, 2018 •

edited

Loading

wuweiweiwu Mar 11, 2018 •

edited

Loading