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

allow setting params to storyshot #4176

Merged

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Sep 13, 2018

Issue: #4119

What I did

Use require('url').URL to parse the storybookUrl. Available since Node v6. (Not sure, what you require here?) Construct the new URL.

How to test

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

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

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@igor-dv
Copy link
Member

igor-dv commented Sep 16, 2018

Can you take a look on the failing tests ?

@donaldpipowitch donaldpipowitch force-pushed the allow-setting-params-to-storyshot branch from e8f8ce6 to b315898 Compare September 17, 2018 06:14
@donaldpipowitch
Copy link
Contributor Author

donaldpipowitch commented Sep 17, 2018

Hi @igor-dv , sorry, I haven't noticed the failing tests. Looks like I covered file: protocol based URLs not correctly. Should be fixed now and added a test for this case.

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

Very cool 👍

const storyUrl = `/iframe.html?selectedKind=${encodedKind}&selectedStory=${encodedStoryName}`;
const { protocol, host, pathname, search } = new URL(storybookUrl);
const pname = pathname.replace(/\/$/, ''); // removes trailing /
const query = search.replace('?', '&'); // convert leading $ to &
Copy link
Member

Choose a reason for hiding this comment

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

// ? to &

@igor-dv igor-dv merged commit c14b250 into storybookjs:master Sep 17, 2018
@donaldpipowitch
Copy link
Contributor Author

Thanks!

@donaldpipowitch donaldpipowitch deleted the allow-setting-params-to-storyshot branch September 17, 2018 09:27
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.

2 participants