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

Fix for issue 5932, adding default options to action(s) #6438

Merged
merged 3 commits into from
Jun 7, 2019

Conversation

LFDanLu
Copy link
Contributor

@LFDanLu LFDanLu commented Apr 5, 2019

Issue: #5932

Apologies for any missteps, first time doing a pull for a open source project :).

What I did

I noticed that both action and actions defaulted their options to {}, hence the reported issue. To remedy this, I imported config from configureActions.ts and set it as the new default for ActionOptions.

How to test

To test, you can play around with the “with text” and “with some emoji” stories in the cra-kitchen-sink example story (https://github.com/storybooks/storybook/tree/next/examples/cra-kitchen-sink).

Sample test flow:

  1. Navigate to “Button” -> “with text” in the cra-kitchen-sink storybook
  2. Click the “Hello Button” a couple times to generate some action logs.
  3. Switch to the “with some emoji” story. Note that the action logs aren’t wiped since that action doesn’t have clearOnStoryChange: true
  4. Click the emoji button in the “with some emoji” story. Note that a new action is logged
  5. Change to a different story. Note that the action log is wiped clean since the action associated with that button doesn’t have any options, thus using the default config from configureActions.

Additional testing scenarios can be done by adding configureActions with various settings in the config.js of the cra-kitchen-sink storybook.

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Apr 5, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-lfdanlu-issue-5932.storybook.now.sh

@LFDanLu LFDanLu changed the title fix for issue 5932, adding default options to action(s) Fix for issue 5932, adding default options to action(s) Apr 5, 2019
@@ -29,67 +28,66 @@ describe('Action', () => {
});
});

// TODO: This functionality is removed, unsure if to add back or remove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that this test was commented out and covers the exact functionality that I was re-adding via this pull. Couldn't find the old implementation nor the reasoning for it's removal so if this was removed for a specific reason I would appreciate a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that found the pull (#4086), but it is quite a large one so I'm still unable to glean the reason for the initial removal. Gonna change my implementation to mirror how user provided config options were merged with the config supplied by configureActions)

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #6438 into next will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6438      +/-   ##
==========================================
- Coverage   41.04%   41.03%   -0.02%     
==========================================
  Files         613      613              
  Lines        8454     8454              
  Branches      378      378              
==========================================
- Hits         3470     3469       -1     
  Misses       4929     4929              
- Partials       55       56       +1
Impacted Files Coverage Δ
addons/actions/src/preview/actions.ts 0% <0%> (ø) ⬆️
addons/actions/src/preview/action.ts 90% <100%> (-10%) ⬇️

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 89ca704...16670df. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #6438 into next will increase coverage by 0.58%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6438      +/-   ##
==========================================
+ Coverage   40.44%   41.03%   +0.58%     
==========================================
  Files         637      613      -24     
  Lines        8759     8454     -305     
  Branches      639      378     -261     
==========================================
- Hits         3543     3469      -74     
+ Misses       5118     4929     -189     
+ Partials       98       56      -42
Impacted Files Coverage Δ
addons/actions/src/preview/action.ts 90% <ø> (-10%) ⬇️
addons/actions/src/preview/actions.ts 0% <0%> (ø) ⬆️
addons/notes/src/Panel.tsx 29.03% <0%> (-8.81%) ⬇️
app/angular/src/server/angular-cli_utils.js 43.75% <0%> (-3.13%) ⬇️
lib/client-api/src/story_store.js 74.57% <0%> (-1.74%) ⬇️
addons/knobs/src/components/Panel.js 80% <0%> (-1.71%) ⬇️
addons/a11y/src/redux-config.tsx 58.33% <0%> (ø) ⬆️
addons/backgrounds/src/constants.ts 0% <0%> (ø) ⬆️
addons/info/src/components/types/PrettyPropType.js 41.66% <0%> (ø) ⬆️
... and 249 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 a52efc7...ad73162. Read the comment docs.

@LFDanLu
Copy link
Contributor Author

LFDanLu commented Apr 8, 2019

@ndelangen I not certain how to assign reviewers, is it supposed to be an automatic process?

@ndelangen ndelangen self-assigned this Apr 26, 2019
@ndelangen ndelangen added this to the 5.2.0 milestone Apr 26, 2019
@stale
Copy link

stale bot commented May 28, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label May 28, 2019
@shilman shilman removed the inactive label May 28, 2019
@ndelangen ndelangen merged commit be5a683 into storybookjs:next Jun 7, 2019
@shilman shilman modified the milestones: 5.2.0, 5.1.x Jun 7, 2019
@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch labels Jun 7, 2019
shilman pushed a commit that referenced this pull request Jun 13, 2019
Fix for issue 5932, adding default options to action(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: actions bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants