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

Addon-viewports: Add back default viewports #7448

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Addon-viewports: Add back default viewports #7448

merged 2 commits into from
Aug 27, 2019

Conversation

ndelangen
Copy link
Member

Issue:

  • viewports no longer had any defaults
  • there was an issue where if the viewport width was > main area, the viewport wouldn't render at full width.

What I did

  • added a minimal default of 3 viewports as a sane list of viewports, hopefully most people will find useful.
  • fixed the layout issue, by using css-grid. flexbox fails for this usecase. This should fail gracefully on browsers that don't support it: they'd just not render the iframe centered.

…wports larger then main area

ADD migration guide to get old defaults again
@vercel
Copy link

vercel bot commented Jul 17, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fix-viewport.storybook.now.sh

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.

@ndelangen Why does this need to be a breaking change? Let's not remove withViewport until 6.0? And what's the problem with just using INITIAL_VIEWPORTS?

@ndelangen
Copy link
Member Author

withViewport is already not exported... Might have happened in the TS migration?

@@ -195,12 +196,15 @@ export const ViewportTool: FunctionComponent<{}> = React.memo(
},
[`#${wrapperId}`]: {
padding: theme.layoutMargin,
display: 'flex',
alignContent: 'center',
alignItems: 'center',
justifyContent: 'center',
Copy link
Member

Choose a reason for hiding this comment

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

if its not a flex container do we still need justify content and align items?

Copy link
Member Author

Choose a reason for hiding this comment

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

display: grid

@stale stale bot added the inactive label Aug 23, 2019
@storybookjs storybookjs deleted a comment from stale bot Aug 26, 2019
@stale stale bot removed the inactive label Aug 26, 2019
@ndelangen ndelangen merged commit 8240849 into next Aug 27, 2019
@ndelangen ndelangen deleted the fix/viewport branch August 27, 2019 08:57
@ndelangen ndelangen changed the title Fix/viewport Add back some default viewports Aug 27, 2019
@shilman shilman changed the title Add back some default viewports Addon-viewports: Add back default viewports Aug 27, 2019
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.

3 participants