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

Viewport addon hidden by default, does not accept defaultViewport parameter #5757

Closed
Jessidhia opened this issue Feb 26, 2019 · 6 comments
Closed

Comments

@Jessidhia
Copy link
Contributor

Describe the bug
@storybook/addon-viewport crashes on initial render, which causes storybooks to render a blank page.

To Reproduce
It seems that all that you need is to specify a defaultViewport:
image

When it is converted by toList, it splits the string:

image

When the list is eventually processed, list.map tries to access named properties but the only one that exists is id:

image

Which eventually crashes inside createItem:

image

Expected behavior
No crash.

Screenshots
It's a blank page, but I added screenshots of the debugger.

System:

  • OS: MacOS
  • Device: MBP
  • Browser: Chrome
  • Framework: Vue
  • Addons: viewport
  • Version: 5.0.0-rc.6
@Jessidhia
Copy link
Contributor Author

Likely broken by a86e25f#diff-0b19096a27d5827867e96b205e01a2ffR13 because it treats the string as an object, copying the enumerable properties.

@Jessidhia
Copy link
Contributor Author

This seems to have been a misconfiguration: https://github.com/storybooks/storybook/tree/next/addons/viewport#change-the-default-viewport

(change from the alphas? or was it just misconfigured all along?)

@Jessidhia
Copy link
Contributor Author

Jessidhia commented Feb 26, 2019

It seems the deprecation message in configureViewport is misleading, so when the message first appeared it was "fixed" by just directly changing configureViewport(options) to addParameters({ viewports: options }). That didn't crash at first, and only started crashing after the commit that I mentioned.

The Viewport tool also does not render unless it is explicitly configured with addParameters({ viewports: INITIAL_VIEWPORTS }), and choosing the default viewport is buggy (it's hardcoded to 'responsive' on mount, only actually looks for the first viewport with default: true if you change stories).

@Jessidhia Jessidhia reopened this Feb 26, 2019
@Jessidhia Jessidhia changed the title [@storybook/addon-viewport] Crash on 5.0.0-rc.6 [@storybook/addon-viewport] Misleading deprecation message Feb 26, 2019
@Jessidhia Jessidhia changed the title [@storybook/addon-viewport] Misleading deprecation message [@storybook/addon-viewport] Docs are incorrect for v5 Feb 26, 2019
@shilman shilman added this to the v5.0.0 milestone Feb 26, 2019
@shilman
Copy link
Member

shilman commented Feb 28, 2019

NOTE: I can repro this in the official storybook in next (5.x) but not in master (4.x)

@shilman shilman changed the title [@storybook/addon-viewport] Docs are incorrect for v5 Addon-viewport: does not show by default, does not accept defaultViewport parameter Mar 3, 2019
@shilman shilman changed the title Addon-viewport: does not show by default, does not accept defaultViewport parameter Addon-viewport: hidden by default, does not accept defaultViewport parameter Mar 3, 2019
@shilman shilman changed the title Addon-viewport: hidden by default, does not accept defaultViewport parameter Viewport addon hidden by default, does not accept defaultViewport parameter Mar 3, 2019
@tmeasday
Copy link
Member

tmeasday commented Mar 4, 2019

Thanks for the report @Jessidhia.

.addParameters({ viewport: 'string' }) will no longer be supported (we need to it to be an object so it can be sensibly merged by the parameters system) but we need to (a) allow setting the default viewport and (b) not throw up if you do the old thing. See #5829

@shilman
Copy link
Member

shilman commented Mar 4, 2019

Yee-haw!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.0-rc.10 containing PR #5829 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Mar 4, 2019
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