Storybook: Please don't use inline styles to set `padding: 1rem` on preview body

Created on 15 Aug 2020  路  12Comments  路  Source: storybookjs/storybook

It makes me use !important in my stylesheet to override it which is not cool

P1 core has workaround maintenance tracked

Most helpful comment

As a workaround you can set the layout story parameter to 'fullscreen' globally in .storybook/preview.js:

export const parameters = { layout: 'fullscreen' };

All 12 comments

As a workaround you can set the layout story parameter to 'fullscreen' globally in .storybook/preview.js:

export const parameters = { layout: 'fullscreen' };

@shilman will it apply some other inline styles instead?

UPD: yes it does. So I still have to use !important to define my own paddings

image

@Hypnosphi can you tell us about your use case? We've made this change in order to standardize how stories (and by extension: snapshots) are displayed. My initial implementation actually allowed you to set a custom padding, but we dropped that after discussing it with the team, for the sake of consistency and simplicity.
By the way you can always add custom padding by wrapping your story in a div or span with padding.

@ghengeveld what I'm asking here is to use a stylesheet with body {...} rules instead of inline styles on body element, to lower the specificity

My usecase is migrating from 5.0 to 6.0 with minimal changes

Yeah I get why a stylesheet is preferable over inline styles for the purpose of overriding them. The reason I'm asking about your use case is so we can perhaps offer you what you're after, without using custom styles. Perhaps an extra option to the layout setting is a valid alternative. Basically what I'm asking is: why do you use a custom padding in the first place?

Because I had them before, and I don't want to re-approve all the screenshots

I think we can do this:
https://github.com/storybookjs/storybook/issues/12041#issuecomment-674398706

Sounds like an improvement to me 馃憤

image
hi, in my case, 1rem padding was added to the top window body instead of iframe body, and export const parameters = { layout: 'fullscreen' }; is not working for that, what is your intention and how can I fix it?

@Hypnosphi Hi, I write a decorator in ./storybook/preview.js and it works.

addDecorator((storyFn, context) => {
  useEffect(() => {
    // Remove body inline style
    document.body.style = null;
  }, []);

  return storyFn(context);
});

Using inline styles has proven problematic. Not only because it requires !important or other hacks to override these styles, it also comes with this annoying "style merging" side effect which causes styles from one layout to carry over to a story with a different layout when switching between stories.

The solution we should implement is to set a data attribute on the body element, and target that data attribute with the appropriate styles. A classname would work as well but a data attribute's value is easier to update than it is to swap classnames. So I propose to set data-layout="padded" on the body by default and update that value according to the layout parameter for the story. Then we can apply a stylesheet with body[data-layout="padded"] { ... } with the appropriate styles for each layout.

If anyone is interested to open a PR for that, please have a go at it!

@ghengeveld #12727 introduces new classes instead. WDYT about that?

Hurrah!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.1.0-alpha.31 containing PR #12727 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

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

Was this page helpful?
0 / 5 - 0 ratings

Related issues

MrOrz picture MrOrz  路  3Comments

dnlsandiego picture dnlsandiego  路  3Comments

wahengchang picture wahengchang  路  3Comments

levithomason picture levithomason  路  3Comments

sakulstra picture sakulstra  路  3Comments