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.

Sign up for GitHub

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

Jump to bottom

Pseudo classes persist when switching stories under load or fast #90

Open
ConcernedHobbit opened this issue Sep 29, 2023 · 6 comments · May be fixed by #119
Open

Pseudo classes persist when switching stories under load or fast #90

ConcernedHobbit opened this issue Sep 29, 2023 · 6 comments · May be fixed by #119

Comments

@ConcernedHobbit
Copy link

Pseudo classes seem to take some time to be removed from the DOM.
I'm running into an issue where switching a story within the same component causes pseudo states from the previous story to still be present.
We can also reliably trigger this with test-runner enabled visual snapshot tests. As the story switches happen very fast, this addon does not react in time to the changes.

This regression was introduced because of a Storybook change sometime between 7.1 and 7.4. I speculate it is because of a race condition with event firing, but do not have time to investigate further.

We've manually cleaned up the leftover classes in visual testing, in test-runner's postRender hook:

if (!parameters.pseudo) {
    const body = await page.$('body');
    await body?.evaluate((node) => {
        const pseudoedElements = node.querySelectorAll('*[class*="pseudo-"]');
        for (const element of pseudoedElements) {
            element.classList.remove(...Array.from(element.classList).filter((c) => c.startsWith('pseudo-')));
        }
    });
}

This occurs before taking a snapshot, and allows us flake-free visual tests.

Let me know if I can assist in debugging! :)
@drewbrend
Copy link

+1

I also hit this in my stories when running visual tests with applitools, but I haven't been able to make a simple reproduction so I didn't open an issue. I've had to disable visual testing on some stories because I haven't been able to resolve this.

@codingedgar
Copy link

It seems to happen when switching from, to and between stories with pseudo, the configuration persists.

@codingedgar
Copy link

It still happens in 3.1.0; it is not an issue with chromatic, only during manual testing.

@codingedgar
Copy link

Downloaded and tested the project, works fine, maybe is an issue with Vite in my configuration?

/// <reference types="vitest" />
import react from '@vitejs/plugin-react';
import { defineConfig } from 'vite';
import sassDts from 'vite-plugin-sass-dts';
import { version } from '../../release/app/package.json';
import { srcRendererPath } from './vite.paths';

export default defineConfig({
  mode: process.env.NODE_ENV === 'production' ? 'production' : 'development',
  root: srcRendererPath,
  cacheDir: 'node_modules/.vite_storybook',
  define: {
    'process.env.BUILD_TYPE': JSON.stringify(process.env.BUILD_TYPE),
    'process.env.VERSION': JSON.stringify(version),
  },
  plugins: [react(), sassDts()],
});

@codingedgar
Copy link

It seems the bug happens when I use CSS Selectors:

parameters = { pseudo: { hover: ["button"] } }

@codingedgar
Copy link

Seems

storybook-addon-pseudo-states/src/preview/withPseudoState.ts

Lines 162 to 172 in cab0bc6

useEffect(() => {
if (!rootElement) return
const timeout = setTimeout(() => {
applyParameter(rootElement, globals || pseudoConfig(parameter))
shadowHosts.forEach(updateShadowHost)
}, 0)
return () => clearTimeout(timeout)
}, [rootElement, globals, parameter])
return StoryFn()
}

runs twice,

first with old parameters and second with new parameters

The first time, it adds pseudo-classes.

If it ran once, with the latest parameters, the problem wouldn't exist, but I'm not sure how to fix that or if it should be ad-hoc fixed here.

Another approach

If there was a way to remove cleanup previous classes before adding new, that would also work, I think.

storybook-addon-pseudo-states/src/preview/withPseudoState.ts

Lines 49 to 79 in cab0bc6

const applyParameter = (rootElement: Element, parameter: PseudoStateConfig = {}) => {
const map = new Map([[rootElement, new Set<PseudoState>()]])
const add = (target: Element, state: PseudoState) =>
map.set(target, new Set([...(map.get(target) || []), state]))
;(Object.entries(parameter || {}) as [PseudoState, any]).forEach(([state, value]) => {
if (typeof value === "boolean") {
// default API - applying pseudo class to root element.
if (value) add(rootElement, `${state}-all` as PseudoState)
} else if (typeof value === "string") {
// explicit selectors API - applying pseudo class to a specific element
querySelectorPiercingShadowDOM(rootElement, value).forEach((el) => add(el, state))
} else if (Array.isArray(value)) {
// explicit selectors API - we have an array (of strings) recursively handle each one
value.forEach((sel) => querySelectorPiercingShadowDOM(rootElement, sel).forEach((el) => add(el, state)))
}
})
map.forEach((states, target) => {
const classnames = new Set<string>()
states.forEach((key) => {
const keyWithoutAll = key.replace("-all", "") as PseudoState
if (PSEUDO_STATES[key]) {
classnames.add(`pseudo-${PSEUDO_STATES[key]}`)
} else if (PSEUDO_STATES[keyWithoutAll]) {
classnames.add(`pseudo-${PSEUDO_STATES[keyWithoutAll]}-all`)
}
})
applyClasses(target, classnames)
})
}

@codingedgar codingedgar linked a pull request May 7, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix pseudo classes persist codingedgar/storybook-addon-pseudo-states
3 participants
@ConcernedHobbit @drewbrend @codingedgar