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

Fix #7167 addon-centered causes component to disappear when zooming #7400

Merged
merged 1 commit into from
Jul 22, 2019
Merged

Fix #7167 addon-centered causes component to disappear when zooming #7400

merged 1 commit into from
Jul 22, 2019

Conversation

8beeeaaat
Copy link
Contributor

@8beeeaaat 8beeeaaat commented Jul 12, 2019

Issue: close #7167

What I did

I resolve the issue #7167 with styling.
The issue is due to the fact that the width and height of the body element are not fit to the iframe.
So I set the style for the body element in iframe.

Before After
before after

How to test

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

@vercel
Copy link

vercel bot commented Jul 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-8beeeaaat-7167-zooming-with-addon-centered.storybook.now.sh

@ndom91
Copy link
Contributor

ndom91 commented Jul 12, 2019

@8beeeaaat Looks good! Now someone on the project can check it out and approve/deny it! 😄

@8beeeaaat
Copy link
Contributor Author

@ndom91
Thank you very much for guiding me!!

@shilman shilman added addon: centered bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jul 12, 2019
@shilman shilman added this to the 5.1.x milestone Jul 12, 2019
Copy link
Member

@CodeByAlex CodeByAlex left a comment

Choose a reason for hiding this comment

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

Tested it locally and it LGTM! Nice work:)

@CodeByAlex CodeByAlex merged commit 688ffe3 into storybookjs:next Jul 22, 2019
@8beeeaaat 8beeeaaat deleted the 7167-zooming-with-addon-centered branch July 23, 2019 00:39
@8beeeaaat
Copy link
Contributor Author

@CodeByAlex
Thank you for your reviewing!!🙌

@CodeByAlex
Copy link
Member

Thank you for contributing! Worked like a charm👌

@aarfing
Copy link

aarfing commented Jul 23, 2019

This change makes the iframe-window too wide as the body has default margin which gets added outside of the 'width: 100%'. So it becomes [margin + 100% + margin] and components which are 100% wide themselves will overflow the iframe.

@CodeByAlex
Copy link
Member

Ah bummer, hadn’t seen that scenario. @aarfing, do you know of a better solution?

@aarfing
Copy link

aarfing commented Jul 23, 2019

@CodeByAlex I guess you could set the body margin explicitly to something like 8px and then do width: calc(100% - 16px)

@CodeByAlex
Copy link
Member

@aarfing do you want to give that a try and put out a PR if it works better? If you do, I can review it

@ndom91
Copy link
Contributor

ndom91 commented Jul 25, 2019

This change makes the iframe-window too wide as the body has default margin which gets added outside of the 'width: 100%'. So it becomes [margin + 100% + margin] and components which are 100% wide themselves will overflow the iframe.

I just tried this myself, and couldn't see any of my full width components overflowing the frame. With or without the sidebar(s).

Are you sure about this? My body and html seem to have margin of 0, see screenshot:

image

@aarfing
Copy link

aarfing commented Jul 26, 2019

@ndom91 @CodeByAlex Looks like you have some resetting CSS, I don't have. This is what I get in 5.2.0-beta.9 having a component that doesn't have its width set, but is just display:block.
Screenshot 2019-07-26 at 08 55 32

@ndom91
Copy link
Contributor

ndom91 commented Jul 26, 2019

@aarfing hmm yeah, so theres this large inline css block at the bottom of the page which resets padding, margin, etc. on a whole bunch of elements:

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
del, dfn, em, img, ins, kbd, q, s, samp,
small, strike, strong, sub, sup, tt, var,
b, u, i, center,
dl, dt, dd, ol, ul, li,
fieldset, form, label, legend,
table, caption, tbody, tfoot, thead, tr, th, td,
article, aside, canvas, details, embed, 
figure, figcaption, footer, header, hgroup, 
menu, nav, output, ruby, section, summary,
time, mark, audio, video {
	margin: 0;
	padding: 0;
	border: 0;
	vertical-align: baseline;
}
/* HTML5 display-role reset for older browsers */
article, aside, details, figcaption, figure, 
footer, header, hgroup, menu, nav, section {
	display: block;
}
...

Im using storybook v5.1.9 btw

@aarfing
Copy link

aarfing commented Jul 26, 2019

@ndom91 I don't think that inline CSS block is part of a clean Storybook, but I might be wrong - this is my first Storybook-project. Anyway, the 'error' was introduced in 5.2.0-beta.6, so you wouldn't be affected by it in 5.1.9.

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 31, 2019
shilman pushed a commit that referenced this pull request Jul 31, 2019
Fix #7167 addon-centered causes component to disappear when zooming
@ndom91
Copy link
Contributor

ndom91 commented Jul 31, 2019

@aarfing okay so I pulled a fresh version and your right. The calc(100% - 16px) fix also works well, but only for the initial zoom value of 0. Each zoom step changes the width of these two elements (html and body), so that zoom + 1 would then need to be calc(75% - 16px), and zoom - 1 would be calc(125% - 16px), etc.

Pull request made: #7640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: centered bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addon Centered causes component to disappear when zooming
5 participants