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.

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

Remove emotion first-child warnings in docs #20730

Closed
tmeasday opened this issue Jan 23, 2023 · 7 comments · Fixed by #21213
Closed

Remove emotion first-child warnings in docs #20730

tmeasday opened this issue Jan 23, 2023 · 7 comments · Fixed by #21213
Assignees

Comments

@tmeasday
Copy link
Member

tmeasday commented Jan 23, 2023

Describe the bug

The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".
@ndelangen
Copy link
Member

I believe this has been fixed..?

@github-project-automation github-project-automation bot moved this from Required for GA to Done in Core Team Projects Feb 6, 2023
@tmeasday
Copy link
Member Author

tmeasday commented Feb 7, 2023

I don't think so @ndelangen:

image

From https://635781f3500dd2c49e189caf-eldrpdmwma.chromatic.com/?path=/docs/storybook-blocks-examples-button--docs (latest build on next).

@tmeasday tmeasday reopened this Feb 7, 2023
@tmeasday tmeasday moved this from Done to Nice to have in Core Team Projects Feb 22, 2023
@tmeasday tmeasday moved this from Nice to have to Required for GA in Core Team Projects Feb 22, 2023
@tmeasday
Copy link
Member Author

tmeasday commented Feb 22, 2023

@ndelangen so what I think has happened here:

  1. Originally there was an issue (in the manager) where our prior use of a special code comment to disable the warning wasn't working any more. This was due to a bug in Emotion.
  2. The workaround for that was to use a special Emotion cache that just didn't send the warning, and removed the need for the special code comments, so we removed them.
  3. Then later, in one PR, @Andarist did the same process as 2. for docs. However, he didn't merge it, as in the meantime:
  4. He fixed the root problem in Emotion, and made a PR to update the version, and suggested reverting step 2.
  5. However, that did't get merged because Emotion got updated separately.

OK, phew! But in all that, we never reverted step 2. And we never either added the cache or added the code comments to docs.

So I believe the right steps forward here are:

  1. Revert step 2 (PR Turn off Emotion's warnings about potentially unsafe pseudo-selectors in SSR #18361)
  2. Add special code comments (ignoreSsrWarning) to whichever docs components are triggering the warnings.

@Andarist
Copy link
Contributor

Both the "what happened" and "what can be done" look OK to me. Let me know if I can assist you anyhow - if the problem won't go away I'm happy to take another look at this.

@tmeasday
Copy link
Member Author

Thanks @Andarist - I'm hoping this should pretty mechanical now I know what's going on! :)

@tmeasday tmeasday moved this from Required for GA to In Progress in Core Team Projects Feb 28, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects Apr 5, 2023
@shilman
Copy link
Member

shilman commented Apr 5, 2023

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

npx sb@next upgrade --prerelease

@shilman
Copy link
Member

shilman commented Apr 12, 2023

Gadzooks!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.3 containing PR #21213 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb@latest upgrade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants