Remove default CSS margin on preview body

[NMinhNguyen]

This brings back the changes from #7742 which seem to have been removed in 4fc3b61

Issue:

What I did

Brought back body margin: 0 changes. I ignored the following changes though, which I think were intentionally removed in #7750.

html, body {
  width: 100%;
  height: 100%;
}

How to test

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

If your answer is yes to any of these, please make sure to include it in your PR.

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/9kq4izvsd
🌍 Preview: https://monorepo-git-fork-nminhnguyen-patch-2.storybook.now.sh

[ndelangen]

@shilman do you agree this is ok?

[shilman]

@ndelangen No, I'm pretty sure it breaks visual regression tests. Unless this is unbreaking something? I'm pretty confused about the history here.

[NMinhNguyen]

FWIW upgrading to Storybook v5 broke my team’s visual regression tests because of the default browser margin being reintroduced. If you follow the discussion in #7742, I think it was agreed that this change is beneficial. Like, removing default browser margins is quite a common CSS reset.

[shilman]

@NMinhNguyen Yeah, there were a couple PRs in 5.1 that modified the preview styles and broke everybody's tests. That was a mistake, and the lesson from that mistake is that preview style changes are actually breaking changes, and should only be introduced on major version bumps. Tens of thousands of teams rely on Storybook, and we should avoid every breaking change we can.

If this is fixing a bug that got introduced in 5.1/5.2, I'm on the fence about merging it.

If this is a breaking change that is "beneficial", I'd propose that we table it until 6.0, even if it's a great change.

[NMinhNguyen]

@shilman Makes sense and sounds good! Luckily, this isn't something that can't be easily applied in userland. Would you be able to tag the PR with 6.0 or something? That is, if you still want to accept this change (eventually).

[shilman]

@NMinhNguyen I'll spend a little time later this week to figure out exactly what happened to see whether this is a bugfix or a breaking improvement. Either way I'd like to merge it--just a question of when.

[ndelangen]

@shilman Do you feel can we commit this to the 6.0.0 release?

[shilman]

@ndelangen if it's the right thing to do, there's no better time to do it and we should merge it into the 6.0 alpha ASAP. can you and @tmeasday please vet this?

[ndelangen]

I talked about this quite a bit with @ghengeveld a while back.

We kinda agreed, that there are some components that are expected to fill the full viewport, and so the page should have no margin.

But these components are in the minority, most components are components that will be composed, and should not have any layout effects. Users probably don't want those <Button /> type components smashed in the top-right corner of the preview, right?
I don't, I like having a bit of padding there.

I was wondering if we should make an core-api or parameter for this purpose:

export default {
  title: 'My Component',
  layout: 'padded', // 'padded' | 'centered' | 'fullscreen'
}

[shilman]

@ndelangen Is that something we can do purely in the manager? Would be nice if story code didn't have to worry about this at all. And I agree this seems like it could be something "fundamental" baked into Storybook.

[ghengeveld]

As @ndelangen and I discussed this recently, we initially came up with a solution to use a configurable margin and background color through addParameters. This works globally and can be overruled locally:

const isScreen = ({ kind, parameters }) => {
  if (parameters.isScreen !== undefined) return parameters.isScreen;
  const [storyRoot] = kind.split(parameters.options.hierarchyRootSeparator || '/');
  return storyRoot === 'screens' || storyRoot === 'layouts';
};

addParameters({
  margin: 0,
  background: context => (isScreen(context) ? '#f6f9fc' : '#fff'),
});

Then in the global config (this could be in core or in an addon):

addDecorator((storyFn, context) => {
  const getValue = value => (typeof value === 'function' ? value(context) : value);
  const pxValue = (value, fallback) => (Number.isInteger(value) ? `${value}px` : fallback);
  const margin = pxValue(getValue(context.parameters.margin), '1rem');
  if (document.body.style.margin !== margin) {
    document.body.style.margin = margin;
  }
  const background = getValue(context.parameters.background);
  if (document.body.style.background !== background) {
    document.body.style.background = background;
  }
  return storyFn();
});

The idea here is that most stories will want to use a white background and 1rem of margin, to provide space for box shadows and such that flow outside the component's bounding box. However, stories for "screens" and "layouts" (which typically don't have any surrounding whitespace) should be rendered with 0 margin and the default page background color. I've implemented it to support a callback so the configuration can be done globally, but be based on the story context.

Additionally, we have the issue of block-level vs inline elements. Wrapping a component in a container in order to apply margin (or other styles) should be avoided because it can't reliably be done globally. If you wrap a component in a div, any visual snapshots will always be 100% with, even for small components, leading really crappy thumbnails. If you wrap it in a span, block-level components will no longer be 100% width. You need knowledge of component internals to determine the correct wrapper element, so it's not something we should ever do by default or through an addon. I think we should (actively) discourage the use of story wrapper elements. We can do that by providing an easy way to control canvas styles, so I think this has a place in core.

[ndelangen]

I think we should (actively) discourage the use of story wrapper elements

I agree

Is that something we can do purely in the manager?

My example/proposal is written in CSF, so it's defined in the preview (and synced to the manager).
How would your proposal look if it were defined in the manager somewhere?

Here's my poposal:

export default {
  title: 'My Component',
  layout: 'padded', // 'padded' | 'centered' | 'fullscreen'
}

And in the preview code we add:

useEffect(() => {
	switch(layout) {
		case 'padded': {
			document.body.style = 'padding: 1rem';
			break;
		}
		case 'centered': {
			document.body.style = 'display: flex; justify-content: center; align-items: center';
			break;
		}
		case 'fullscreen':
		default: {
			document.body.style = '';
			break;
		}
	}
}, [layout])

[ndelangen]

This would remove the need for addon-centered too, making it a core feature

[tmeasday]

I think we need to give this some more thought but on the surface it makes sense that there is a dichotomy between how you want to treat "component" vs "screen" stories, and it would make sense to have a first-class concept for that. Certainly in Chromatic we've discussed that it'd be visually nice to capture screenshots of components with a bit of margin but screens without.

Technically there is three ways (I can think of) to achieve the margin:

1. Wrap the story in a wrapping div.
2. Add margin/padding to the body.
3. Pad the actual iframe in the manager.

• wrapping the story:
  • is kind of messy (adding extra divs)
  • also is tricky with layout (block vs inline-block etc).
• doing it in the manager could be problematic when it comes to drop-shadows et al.

So I think basically controlling the default body padding per story is the way to do it.

I'm not sure about the centered part @ndelangen -- seems like an orthogonal concern.

[ghengeveld]

Controlling the margin/padding on body is the right way to do it. Wrapper elements should be avoided as discussed above and I think setting padding on the iframe will hide overflowing content (e.g. cut off shadows).

I really like Norbert's proposal because it naturally tackles the problem we're trying to solve without specifying implementation details, and it standardizes settings across the ecosystem (i.e. it avoids everyone coming up with their own default padding).

I'm not 100% sure about the centered setting, but I suppose it's common enough to warrant it being in core. I do think it should also have the 1rem padding for responsiveness and snapshotting. I'm also not sold on the naming for the layout options. Maybe position: default|full|center?

We have to update Chromatic to take body padding into account, but that's another story.

[stale]

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