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

Removes default CSS margins on viewport #7742

Merged
merged 4 commits into from
Sep 24, 2019

Conversation

quentind
Copy link
Contributor

Related to issue #7724 and PR#7640

What I did

This resets the default CSS margins on the body element. This prevents having vertical and horizontal scrollbars on the preview viewport as is the case in version 5.1.10, for instance.

This also fixes a regression on the latest version compared to the live examples running on 5.1.3, where there is no whitespace around the preview area.

This 8px whitespace -- the browsers' default margin on the body -- around the preview area is not desirable in my opinion, especially when the previewed component has a width of 100% or is simply a block element.

Resets default CSS margins on the body element. 
This prevents a regression compared to live example running on 5.1.
@vercel
Copy link

vercel bot commented Aug 12, 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-fork-quentind-patch-issue-7724.storybook.now.sh

@shilman
Copy link
Member

shilman commented Aug 12, 2019

@quentind How does this affect the centered addon zooming behavior that the original PR was trying to fix? Regardless, this fix is more important than the original PR, but it would be great to know whether we'll need to revisit that issue. Thanks!

@quentind
Copy link
Contributor Author

@shilman I'm not experiencing any issues with the centered addon on 5.1.10 + the changes I've suggested.

My understanding is that the changes from PR#7640 can be fully reverted actually since they were simply accounting for the margin on the <body> which is removed by my PR.

So this line can be reverted back to:

- width: `calc(${nextProps.scale * 100}% - 16px)`,
+ width: `${nextProps.scale * 100}%`,

Thank you for the feedback.

@shilman
Copy link
Member

shilman commented Aug 12, 2019

@quentind can you add the change you suggest in this PR?

@quentind
Copy link
Contributor Author

@shilman Done! Thanks

@shilman
Copy link
Member

shilman commented Aug 12, 2019

Thanks @quentind. Waiting to merge this until we figure out the implications to visual regression tools that depend on this.

@ndelangen ndelangen self-assigned this Aug 19, 2019
# Conflicts:
#	lib/core/src/server/templates/base-preview-head.html
@ndelangen ndelangen merged commit 587a794 into storybookjs:next Sep 24, 2019
@shilman
Copy link
Member

shilman commented Sep 24, 2019

@ndelangen why does this PR not show any changed files?

@ndelangen
Copy link
Member

Because after merging in next, there were none. I could have just closed the PR, but I think merging makes for a better indication that @quentind contributed, and that's appreciated.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Oct 9, 2019

@ndelangen having installed 5.3.0-alpha.15 and looking at next branch, I have a suspicion that this change never made it in:

4fc3b61 (commit that seems to have been due to a merge conflict, and you can see code removed)

https://github.com/storybookjs/storybook/blob/1d517d0/lib/core/src/server/templates/base-preview-head.html (next branch without those lines of code)

Update

I've raised #8358 to restore these changes

NMinhNguyen added a commit to NMinhNguyen/storybook that referenced this pull request Oct 9, 2019
This brings back the changes from storybookjs#7742
which seem to have been removed in
storybookjs@4fc3b61
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.

4 participants