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

Feature: Extend setAddon API #1463

Closed
wants to merge 4 commits into from
Closed

Feature: Extend setAddon API #1463

wants to merge 4 commits into from

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Jul 13, 2017

The current API:

setAddon({ addonA(), addonB() }) 

It adds addons to storiesOf() object and they accessible as methods:

storiesOf(...)
  .addonA()
  .addonB()

Why we need new API

Sometimes we want to implement some features or some behavior to storiesOf() to have it by default. This would open up a plenty new directions in the addons creation!

What I did

Just allow passing into setAddon functions matching /^_init/ and make them executed right after storiesOf() is created.
This is a backward compatible change, addons with normal names still work the same.

New API

setAddon({ addonA(), addonB(), _initSmth() }) 

when you create:

storiesOf(...)
  .addonA()
  .addonB()

_initSmth() will not be accessible as a method of storiesOf() but it will be already executed

How to test

run cra-kitchen-sink
check out the Toc first story of each storyKind (it just an example of how it works)
check that other addons work as usual

@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #1463 into release/3.3 will decrease coverage by 6.39%.
The diff coverage is 77.77%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release/3.3    #1463     +/-   ##
==============================================
- Coverage        22.32%   15.93%   -6.4%     
==============================================
  Files              324      237     -87     
  Lines             6319     5051   -1268     
  Branches           803      629    -174     
==============================================
- Hits              1411      805    -606     
+ Misses            4297     3715    -582     
+ Partials           611      531     -80
Impacted Files Coverage Δ
app/react/src/client/preview/client_api.js 83.33% <77.77%> (-2.39%) ⬇️
...s/stories/required_with_context/Welcome.stories.js 0% <0%> (-100%) ⬇️
...dons/storyshots/stories/directly_required/index.js 0% <0%> (-100%) ⬇️
...ts/stories/required_with_context/Button.stories.js 0% <0%> (-100%) ⬇️
addons/info/src/index.js 0% <0%> (-90.91%) ⬇️
addons/storyshots/src/index.js 0% <0%> (-80.86%) ⬇️
lib/ui/src/modules/ui/components/search_box.js 16.66% <0%> (-77.28%) ⬇️
addons/info/src/components/Story.js 0% <0%> (-76.2%) ⬇️
lib/ui/src/modules/ui/containers/search_box.js 33.33% <0%> (-66.67%) ⬇️
addons/info/src/components/markdown/text.js 0% <0%> (-66.67%) ⬇️
... and 226 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 f199465...64340bd. Read the comment docs.

@usulpro usulpro changed the title Set addon extention Feature: Extend setAddon API Jul 13, 2017
@shilman
Copy link
Member

shilman commented Jul 13, 2017

Super interesting idea. However, personally I'd vote against expanding the API at all until we get the existing API working better.

@usulpro usulpro changed the base branch from release/3.2 to release/3.3 September 14, 2017 10:11
@usulpro
Copy link
Member Author

usulpro commented Sep 14, 2017

This approach can help us to get rid of global decorators:

Instead of adding them globally addDecorator can add them after storyKind is created

@danielduan
Copy link
Member

Closing this because there hasn't been any activity here for a while. Feel free to reopen when this is ready for discussion again.

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.

None yet

4 participants