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

Send story context to transform source #12327

Conversation

NikkiTheBugSlayer
Copy link

Issue:

What I did

This is similar to #12265 , but sends the context to transformSource as third argument so it is not a breaking change. This is useful for updating the source block when a users changes the controls.

How to test

I created the button-with-custom-source-code story to demonstrate how context can be used in transformSource. I also updated enhanceSource to test everything was being sent correctly.

const { source } = baseContext.parameters.docs;
const parameters = { ...baseContext.parameters, docs: { source, transformSource } };
expect(enhanceSource({ ...baseContext, parameters })).toBeNull();
});
it('receives StoryContext as second argument', () => {
Copy link
Member

@shilman shilman Aug 31, 2020

Choose a reason for hiding this comment

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

can we test this with a single test instead of three?

Copy link
Author

Choose a reason for hiding this comment

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

I removed the extra tests.

Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

It's not great that this API will have a different signature than jsxDecorator, but I understand if @shilman might think changing the 2nd argument will actually break people.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @UX-Unicorn! I prefer @phated 's breaking solution -- this API is brand new I don't think enough people are using it yet, so I'm going to make a semver exception and merge that PR instead. Sorry for making you update the tests, I didn't fully understand the PR until I'd read all the related PRs at once.

@NikkiTheBugSlayer
Copy link
Author

Thanks for this contribution @UX-Unicorn! I prefer @phated 's breaking solution -- this API is brand new I don't think enough people are using it yet, so I'm going to make a semver exception and merge that PR instead. Sorry for making you update the tests, I didn't fully understand the PR until I'd read all the related PRs at once.

@shilman That makes sense. I just didn't want to have to wait for the breaking change to get merged to get access to the context in transformSource.

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

3 participants