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

Fixed knobs-addon performance issues #1039

Merged
merged 3 commits into from
May 25, 2017
Merged

Fixed knobs-addon performance issues #1039

merged 3 commits into from
May 25, 2017

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented May 16, 2017

Issue: #809 Add throttling/batching of updates for addon-knobs.

Fixing performance issues

Added timestamps to setKnobs event.

If user is typing, his typing shouldn't be interrupted by randomly happening setKnobs action. Now setKnobs will actually do something only after user finishes typing.

Example

const {withKnobsOptions} = require('storybook-addon-knobs');
story.addDecorator(withKnobsOptions({timestamps: true}));

Added ability to pass debounce option when adding decorator

As #92 said we needed an option to add debouncing to knob changes.
I added an option to pass options when adding decorator to story.

Example

const {withKnobsOptions} = require('storybook-addon-knobs');
//debounce options are wait and leading, same as in lodash.
story.addDecorator(withKnobsOptions({debounce: { wait: 100, leading: true}}));

Difference between these two updates

The first update timestamps solves issues that happen when developing locally, since sometimes setKnobs happened after user has already started typing and it reverted the typing.
The debounce is useful if you are using knobs over the air, in device or in separate server, it simply doesn't send so many requests.

Of course you can use both at the same time:

const {withKnobsOptions} = require('storybook-addon-knobs');
//debounce options are wait and leading, same as in lodash.
story.addDecorator(withKnobsOptions({
    debounce: { wait: 100, leading: true},
    timestamps: true
}));

Things to note

  • Added loadash.debounce package as a dependency.
  • Two tests changed, added timestamps so they would work.

How to test

See examples above.

@Gongreg Gongreg changed the title Fixing performance issues. [Addon-Knobs] Fixing performance issues. May 16, 2017
@codecov
Copy link

codecov bot commented May 16, 2017

Codecov Report

Merging #1039 into master will increase coverage by 0.15%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   12.74%   12.89%   +0.15%     
==========================================
  Files         199      199              
  Lines        4472     4489      +17     
  Branches      711      714       +3     
==========================================
+ Hits          570      579       +9     
- Misses       3272     3278       +6     
- Partials      630      632       +2
Impacted Files Coverage Δ
addons/knobs/src/index.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/WrapStory.js 11.11% <0%> (-0.16%) ⬇️
addons/knobs/src/KnobManager.js 39.13% <50%> (+0.32%) ⬆️
addons/knobs/src/components/Panel.js 88% <85.71%> (-4.31%) ⬇️

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 41cbc3a...ad08ab3. Read the comment docs.

@ndelangen
Copy link
Member

@Gongreg do you need help rebasing?

@danielduan
Copy link
Member

+1 lgtm, looks like it would definitely help for props that trigger big dom changes. thanks for the PR @Gongreg !

@Gongreg
Copy link
Member Author

Gongreg commented May 22, 2017

@ndelangen Just like we talked in slack, if you could rebase it would be cool :)

@ndelangen
Copy link
Member

I will rebase this for you.

@ndelangen ndelangen self-assigned this May 22, 2017
@ndelangen ndelangen added this to the v3.1.0 milestone May 24, 2017
@ndelangen
Copy link
Member

rebased !

@danielduan
Copy link
Member

danielduan commented May 24, 2017

I'm in favor of fixing the lint issues: var => const, extra parenthesis, etc. Otherwise, I think its good to go.

Actually scratch that, they're already existing so don't worry about it. Let's get this in the next alpha?

@ndelangen ndelangen merged commit 0a81bc6 into storybookjs:master May 25, 2017
@ndelangen ndelangen changed the title [Addon-Knobs] Fixing performance issues. Fix performance issues for knobs addon May 27, 2017
@ndelangen ndelangen modified the milestones: v3.0.0, v3.1.0 May 27, 2017
@shilman shilman changed the title Fix performance issues for knobs addon Fixed knobs-addon performance issues May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants