rbardini / storybook-addon-paddings

🔲 A Storybook addon to add different paddings to your preview
https://storybook-addon-paddings.rbrd.in
MIT License
30 stars 2 forks source link

Padding seems broken in docs mode for HTML framework project as of 4.2.0 #28

Closed tylersticka closed 1 year ago

tylersticka commented 3 years ago

When updating to 4.2.0, all of our stories displayed errors complaining of a duplicate decorator and a missing fn for the withPaddings decorator.

Viewing the source, it seemed like the need to manually enable the decorator was removed, so we removed that from our preview.js file. This fixed the errors, but in docs mode all of the padding from our stories was set to 0.

Screen Shot 2021-07-09 at 10 07 32 AM

Here's a live example: https://v-next--cloudfour-patterns.netlify.app/?path=/docs/components-button--button-element

What's interesting is that if I peer into the styles of the element I'd expect to have padding, I see the following inline styles:

      .sb-show-main {
        margin: 0;
        padding: 1em !important;
        transition: padding .3s;
      }

      .sb-show-main .innerZoomElementWrapper > div {
        border-width: 0 !important;
      }

I wouldn't expect .sb-show-main to be the selector here, based on this portion of this project's source: https://github.com/rbardini/storybook-addon-paddings/blob/33a4bf6fa7af7d9989e9df157ddee66ea27cc10f/src/index.tsx#L27-L29

I also tried adding the decorator again manually using the updated export name (WithPaddings). This restored Storybook's default padding but appears to have broken the add-on (I see duplicate errors in the console).

Our project is public if it's helpful to try running it locally: https://github.com/cloudfour/cloudfour.com-patterns

rbardini commented 3 years ago

Hi @tylersticka! Indeed, in docs mode the addon should use the following selector:

https://github.com/rbardini/storybook-addon-paddings/blob/33a4bf6fa7af7d9989e9df157ddee66ea27cc10f/src/index.tsx#L28

I can see that the styles are being applied to the iframe:

Styles

However, your .docs-story element somehow is not a child of #anchor--${id}:

DOM structure

Do you have any idea why? Maybe that's something the addon should handle, but the implementation is based on the official Backgrounds addon, so I'd expect it to be pretty solid. You could also try the Backgrounds addon and check if it works 🙂

tylersticka commented 3 years ago

@rbardini This seems to be a problem with the official Backgrounds add-on as well: https://github.com/storybookjs/storybook/issues/14322

The issue is that "a single Anchor component at the top of the page for the first Story encountered," which invalidates the selector that they are using.

(We ended up rolling our own theme decorator because of this. )

rbardini commented 3 years ago

I wasn't aware of that issue, thanks for bringing it up! I think we could fix this issue with a sibling selector like:

`#anchor--${id} ~ .sbdocs-preview .innerZoomElementWrapper`

I'll add MDX stories to the example and check if that would work.

rbardini commented 3 years ago

I opened #29 with a fix (see example), but noticed yet another MDX quirk in docs mode:

If I switch to canvas and back to docs, the #anchor--${id} element just disappears, and the selector doesn't match anymore. Unless I select a padding option, then Storybook renders the anchor again and things work as expected.

Another downside of this approach is that we lose story parameters specificity, as mentioned in https://github.com/storybookjs/storybook/issues/14322#issuecomment-808358465. Ideally, Storybook should normalize the docs output and wrap every MDX <Canvas> with an <Anchor>.

tylersticka commented 3 years ago

Ugh, that's frustrating!

Any idea why this worked more consistently before 4.2.0? Maybe for our project we should consider downgrading the add-on for now.

rbardini commented 3 years ago

v4.2.0 changed how paddings are applied, replacing inline styles with class names, following the backgrounds addon implementation. The motivation was to make it work in docs mode. Ironically, it seems to have broken MDX in docs mode—I never checked it before, so if it did work it was by sheer luck 😄

I might merge #29 anyway, as partial support is likely better than no support at all, but I'm afraid this issue can only be truly fixed in Storybook.

tylersticka commented 3 years ago

I appreciate you taking a look at this, @rbardini! I know open source can be rather thankless but I appreciate the thoughtful compromise.