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

Options set via addon should not override URL params #1690

Closed
Hypnosphi opened this issue Aug 20, 2017 · 12 comments
Closed

Options set via addon should not override URL params #1690

Hypnosphi opened this issue Aug 20, 2017 · 12 comments
Assignees

Comments

@Hypnosphi
Copy link
Member

  1. Run yarn start in repo root
  2. Toggle actions panel with ⌘ ⇧ D
  3. Reload the page

Expected: Actions panel stays hidden
Actual: It is hidden for a moment, and then shows up

@usulpro
Copy link
Member

usulpro commented Aug 20, 2017

@Hypnosphi it happens because we use setOptions in config.js to setup initial state.
If you remove setOptions it'll behave as you describe

At least I don't think that it's a bug, or I missed something?

@Hypnosphi
Copy link
Member Author

My point is that query params should have higher priority when present

@usulpro
Copy link
Member

usulpro commented Aug 20, 2017

I see but setOptions can be used per story to setup appearance accordingly to selected story. It'll stop working if we change the priority

@Hypnosphi
Copy link
Member Author

Well, if I reload the page, I stay on the same story. Links to other stories can be adjusted according to their specific setOptions

@usulpro
Copy link
Member

usulpro commented Aug 20, 2017

So you offer to detect where it runs: inside a story or not? And depending on that change setOptions behavior?

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Aug 20, 2017

I'd suggest roughly that:

  1. explicit URL param has precedence when defining current view
  2. hiding/showing a panel changes the current page URL
  3. URL/action params in links to other stories are formed in this manner:
    a) If the story has set the corresponding option via local setOptions, use that
    b) Otherwise, use the current value

@Hypnosphi
Copy link
Member Author

By the way, how do you use setOptions per story? Can't find that in docs

@usulpro
Copy link
Member

usulpro commented Aug 20, 2017

this way:
https://github.com/storybooks/storybook/blob/master/examples/cra-kitchen-sink/src/stories/index.js#L83

or even like this:
https://github.com/storybooks/storybook/blob/master/examples/cra-kitchen-sink/src/stories/index.js#L70

so in this cases URL could be formed twice:

  1. when you select a story in stories panel
  2. when this story trigger setOptions and it updates query params

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Aug 20, 2017

Oh, so it's inside story render. Ok, I think my suggestion may be restricted to setOptions in global setup. I think it would be enough to run initApi before handling routing (and setting only the explicitly present params in the latter)

@stale
Copy link

stale bot commented Nov 14, 2017

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 60 days. Thanks!

@stale stale bot added the inactive label Nov 14, 2017
@Hypnosphi
Copy link
Member Author

I think it’s still an issue worth fixing

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Apr 1, 2018

Will fix it as part of #2313

@issue-sh issue-sh bot removed the todo label Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants