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

[Feature request] opt-out and/or configure padding transition/animation duration #31

Open andyford opened 2 years ago

andyford commented 2 years ago

I'm wondering if it could be possible to opt-out of (or configure) the padding CSS animation duration.

Subjectively, I personally find it a little distracting that the padding animates on each new page view.

Objectively, I've encountered an issue with the visual regression testing approach outlined here https://storybook.js.org/blog/interaction-testing-with-storybook/#extend-and-customize-using-the-test-hook-api ... my screenshots were inconsistent and it turned out that it was caused by the paddings addon.

For now, I've added .sb-show-main { transition: none; } which is fine but the Storybook team seems to be moving more toward this visual regression testing approach (rather than Storyshots), so this issue may come up more often.

rbardini commented 2 years ago

Hi @andyford! That makes total sense. The CSS transition was based on the Backgrounds addon behavior, although I noticed it has a check for reduced motion preference now.

Do you know if adding that check would solve the visual regression testing issue? In other words, does the test framework load pages with reduced motion preference?

It wouldn't get rid of padding transitions on each story change though, so opting out or completely removing the feature are still considered 🙂

andyford commented 2 years ago

Sorry I haven't been able to test this but I can't imagine relying on a user setting like prefers-reduced-motion would be very reliable especially in automated contexts. Also window may possibly be problematic depending on the environment (window.matchMedia('(prefers-reduced-motion: reduce)')).

But to answer the question does the test framework load pages with reduced motion preference? ... I guess if it did by default, then I never would have encountered this problem =) Also I think the way the testing approach in the article is handled is via a headless browser, so I'm not sure you really have much of a chance to set the prefers-reduced-motion setting anywhere.

But as an accessibity consideration, it would be super nice to do the prefers-reduced-motion check. This can be done in pure CSS https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion but it's not clear to me how to integrate the CSS media query into the abstract css-in-js syntax here https://github.com/rbardini/storybook-addon-paddings/blob/898b95d17d66364d0b01e3176ebef81abe353d59/src/index.tsx

rbardini commented 2 years ago

That code has to run in a browser context, so I think it's safe to assume window is defined. Anyway, a media query could also be used, as the CSS in just injected into a style element.

Even if the test framework load pages with reduced motion preference, you would still encounter this problem, as the addon currently doesn't disable the transition in that case. But I agree we may not rely on that preference being set, especially if there's no way to configure it.

So, we should handle reduced motion for accessibility reasons, but it may not solve visual regression testing issues, and won't stop transitions from replaying when switching between stories. (relates to https://github.com/storybookjs/storybook/issues/17122)

I'll probably submit a couple separate PRs for these. Until then, the workaround is to turn off transitions in the preview 😉