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

Provide a configuration option to limit the number of actions logged #3447

Merged

Conversation

jcharry
Copy link

@jcharry jcharry commented Apr 18, 2018

Issue: The actions logger panel accumulates actions for a story endlessly. After a while, this creates quite a bit of lag. You can hit the clear button to clear out the logger and performance improves again, but it'd be nice to have a configuration hook to set a hard upper bound on the number of actions that should be logged.

What I did

Added a configuration option called limit to the configureActions call.

@jcharry jcharry requested a review from rhalff as a code owner April 18, 2018 19:52
@rhalff rhalff requested a review from Hypnosphi April 18, 2018 20:48
@rhalff
Copy link
Contributor

rhalff commented Apr 18, 2018

A default limit could probably be added to configureActions, not sure what the default value should be though.

I've added @Hypnosphi as reviewer, does the PR also need to include an example in the official-storybook?

@danielduan
Copy link
Member

danielduan commented Apr 18, 2018

I think we should set a sensible default for all users while we're making this change. This seems like an obvious optimization we can make on behalf of all users.

does 50 sound like a good number?

@Hypnosphi
Copy link
Member

does the PR also need to include an example in the official-storybook?

Yeah, that wouldn't hurt

I think we should set a sensible default for all users while we're making this change. This seems like an obvious optimization we can make on behalf of all users.

does 50 sound like a good number?

Makes sense to me

@jcharry
Copy link
Author

jcharry commented Apr 18, 2018

Sure, I'll make those additions.

@codecov
Copy link

codecov bot commented Apr 18, 2018

Codecov Report

Merging #3447 into master will increase coverage by 55.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #3447       +/-   ##
==========================================
+ Coverage   37.42%   92.5%   +55.07%     
==========================================
  Files         455       6      -449     
  Lines       10296      40    -10256     
  Branches      911       2      -909     
==========================================
- Hits         3853      37     -3816     
+ Misses       5915       2     -5913     
+ Partials      528       1      -527
Impacted Files Coverage Δ
lib/ui/src/modules/ui/configs/handle_keyevents.js
app/mithril/src/server/config/babel.prod.js
addons/knobs/src/angular/helpers.js
addons/events/src/components/Event.js
addons/storysource/src/loader/index.js
...tions/src/lib/types/function/createFunctionEval.js
app/react-native/src/index.js
app/mithril/bin/build.js
addons/background/src/mithril.js
addons/actions/src/lib/types/nan/index.js
... and 437 more

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 b818f14...578c575. Read the comment docs.

@danielduan
Copy link
Member

@jcharry can you fix the failing test here?

yarn bootstrap --core and then yarn test --core --update

@jcharry
Copy link
Author

jcharry commented Apr 23, 2018

@danielduan done!

@Hypnosphi
Copy link
Member

Hypnosphi commented Apr 23, 2018

Please add a usage example to examples/official-storybook

@jcharry jcharry requested a review from usulpro as a code owner April 25, 2018 16:59
@jcharry
Copy link
Author

jcharry commented Apr 25, 2018

@Hypnosphi Okay, added an example.
EDIT: Oops, now there are some conflicts. Let me resolve those.

@jcharry jcharry force-pushed the jcharry/limit-action-logger-items branch from d2c8c01 to e5766e4 Compare April 25, 2018 19:17
@jcharry jcharry force-pushed the jcharry/limit-action-logger-items branch from e5766e4 to 578c575 Compare April 25, 2018 20:02
@jcharry
Copy link
Author

jcharry commented Apr 25, 2018

Rebased and should be good to go now.

@Hypnosphi Hypnosphi merged commit e83e9ea into storybookjs:master Apr 25, 2018
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