Docs: using targetted styles, not components to style MDX #19958

tmeasday · 2022-11-25T02:30:35Z

Issue: Our MDX components no longer work for pure HTML in MDX, in particular our Introduction.mdx.

Current:

6.x

The root cause here is a breaking change in MDX2: https://mdxjs.com/migrating/v2/#update-mdx-content

If you put down (as we do in Introduction.mdx):

<div className="subheading">Configure</div>

It will no longer be replaced by the component you specify for <div> to MDX. Currently we pass a "styled div" that (in the case of the <div>) does a reset and sets fonts etc. This will not work in MDX2.

There are a couple of problems:

Our introduction's styles are messed up.
Any user that has relied on our styling for raw HTML in MDX is going to have an issue.

What I did

@JReinhold and I have prototyped a solution using targetted styles on the DocsContent wrapper. Basically for the above, we use:

div:not(.sb-story div) { /* styles */ }

To target styles at all <div> elements that are not children of .sb-story (we will attach that style to the output of the <Story/> block). That ensures the styles apply inside the MDX but not inside stories.

However, there are some issues with this approach:

Some of the components do more than just styles, primarily the <code>. In this case, we won't be able achieve full parity.
It seems like MDX is inserting some extra <p> elements that change the structure of the generated HTML anyway. I'm not sure if this is a bug, but this will likely mess people up too.
The specificity of our styles change as a result -- before it was .css-2ltd27 (some emotion class), now it it .css-2ltd27 div:not(.sb-story div) which is a higher specificity, which is going to be a breaking change (in particular we need to retarget the inline styles in Introduction.mdx to ensure they override the reset).
The styles will target other elements inside components the user uses in MDX, such as our doc blocks, or 3rd party "blocks".

Other options

Some other alternatives we've considered:

Alternative 1: Simply change the introduction to look nice (by adding more styles) and make raw HTML in MDX completely unstyled.

Alternative 2: As above, but include a very basic set of styles (e.g fonts, line-height) on the wrapper element, let it cascade, then reset it on the <Story/> element.

Other notes

Due to 2-3, I believe it's not going to be possible for this not to be a breaking change to some degree for users using raw HTML. We might take the opportunity to simplify things a little (i.e. do alternative 2).

JReinhold · 2022-11-30T13:44:40Z

@tom There's one minor issue with our opt-out approach, namely style inheritance. Some styles are inherited by parents even if they are not set.
Eg. adding color: green to a root div will cause all descendants to have color: green, (where as the same thing is not true for border, not all descendants will have a border just because an ancestor has one).

With our global selector, this means that styles can get inherited even through an <Unstyled> or <Canvas> component.

Examples:

<Unstyled>     <-- blocks our global styles, this is great, it works
  <div>
    {'this is text'}
    <Story of={StoriesMeta.Undefined} meta={StoriesMeta}/>
  </div>
</Unstyled>

<div>     <-- gets styled by our global styles
  {'this is text'}
  <Unstyled>     <-- blocks global styles for all descendants, but not the parent div 
    <Story of={StoriesMeta.Undefined} meta={StoriesMeta}/>     <-- will inherit the global styles from the div, even though it doesn't have them directly
  </Unstyled>
</div>

if our global styles styled div with color: green, the above would output this:

Solutions:

This might be a minor thing, that we could "ignore". I expect it would mostly be an issue with inheriting our font family and size.
We could document that Unstyled, Canvas and Story will not behave as intended unless they are added to the root markdown
I've tried with the new :has selector to select parents, so that the second example would also select the ancestor div and make it unstyled, and it works, but I'm afraid this would be unexpected behavior for the user? It is also still not supported in Firefox (it will break the whole global style, not just ignore the :has part).

JReinhold · 2022-11-30T13:46:27Z

Also, I looked into the few components that still used the components from @storybook/components, and it wasn't straight forward to yank them out. I still think it's important that we do it so the styles don't go out of sync, but in the interest of time I've decided to postpone that work.

JReinhold · 2022-11-30T13:47:08Z

code/ui/blocks/src/blocks/Unstyled.mdx

@@ -0,0 +1,39 @@
+import { Unstyled } from './Unstyled.tsx';


@jonniebigodes if you have feedback for this documentation, let me know.

JReinhold · 2022-11-30T13:50:06Z

code/ui/blocks/src/components/DocsPage.tsx

+  '.sb-unstyled',
+  '.sbdocs-preview',
+  '.sb-story',
+  '.docblock-source',
+  '.sb-anchor',


It's up for discussion how the responsibility should be divided here.

This selector could also just be sb-unstyled, and then it was the responsibility of any of the components (Story, Canvas, etc.) to add that class to their elements.

Who is responsible for unstyling elements, the styler (here) or the element?

I'm open to whatever but my instinct was the other way round (.sb-unstyled)

Let's keep it as-is then and revisit if we get wiser.

Narrator: He did get wiser. #21150

JReinhold · 2022-11-30T13:51:37Z

code/ui/blocks/src/components/DocsPage.tsx

@@ -40,9 +63,374 @@ export const Subtitle = styled.h2(withReset, ({ theme }) => ({
  color: transparentize(0.25, theme.color.defaultText),
 }));



The following style are copied from all the components in https://github.com/storybookjs/storybook/tree/tom/sb-996-resolve-css-issue-with-introduction/code/ui/components/src/typography/elements

tmeasday · 2022-12-01T00:04:45Z

@JReinhold I get it, I think that limitation makes sense and is an issue in the 6.x implementation as well, right? I think we can live with it for sure.

JReinhold · 2022-12-01T00:08:26Z

is an issue in the 6.x implementation as well, right?

Huh, you must be right, I hadn't thought about that at all. No problem then.

tmeasday

Awesome @JReinhold! Can we add some notes about breaking change to the MIGRATION.md (cc @shilman / @jonniebigodes we should maybe mention in MDX2 docs too):

styles leak into non-story components if you don't wrap in <Unstlyed>
<Code> needs to be imported now

code/ui/blocks/src/components/DocsPage.stories.tsx

tmeasday · 2022-12-01T00:09:46Z

@JReinhold I can't approve as I opened this PR, but I'm happy with this.

JReinhold · 2022-12-01T15:00:15Z

@tmeasday I've added some entries to the migration doc, please check it out.

<Code> needs to be imported now

This is actually not possible currently, because the component that replaces code isn't exported to the user. I've tried:

using Code from @storybook/components, but that it doesn't look quite the same, the font is not the same size at least.
using Source from @storybook/blocks but that doesn't work without a Meta tag, so I don't see that as an option
exporting and using Source from @storybook/blocks/components but it's very unergonomic.

As you can see from my migration notes, I think the use cases for defining a code block with <code> are diminishing with MDX2's better support for code fences, so maybe we don't need a mitigation step for this at all? It may only be when a user wants to add custom classes/ids to their code blocks that it would be useful to them.

tmeasday · 2022-12-01T23:03:40Z

I'm happy with that, that's great!

POC: using targetted styles, not components to style MDX

c0eef1a

tmeasday added bug addon: docs labels Nov 25, 2022

tmeasday requested review from shilman, ndelangen, JReinhold and yannbf November 25, 2022 02:30

ndelangen added 2 commits November 25, 2022 10:15

Merge branch 'next' into tom/sb-996-resolve-css-issue-with-introduction

feae2e3

fix types

23d3452

ndelangen approved these changes Nov 25, 2022

View reviewed changes

JReinhold added 5 commits November 29, 2022 21:19

migrate all custom component styles to global styles

b0fa9d7

Move styles to DocsContent, add .sb-story to Story block

5e4226d

build Unstyled block, prettier ignore mdx

5291762

fix docsPage stories getting global styles

b3baca1

make Description not use styled components

2820c5a

JReinhold reviewed Nov 30, 2022

View reviewed changes

cleanup Unstyled docs

7d7777a

JReinhold reviewed Nov 30, 2022

View reviewed changes

JReinhold marked this pull request as ready for review November 30, 2022 13:58

JReinhold changed the title ~~Docs: POC: using targetted styles, not components to style MDX~~ Docs: using targetted styles, not components to style MDX Nov 30, 2022

JReinhold self-assigned this Nov 30, 2022

tmeasday commented Dec 1, 2022

View reviewed changes

code/ui/blocks/src/components/DocsPage.stories.tsx Outdated Show resolved Hide resolved

migration notes.

3c3ccba

Merge branch 'next' into tom/sb-996-resolve-css-issue-with-introduction

7e2d3f4

JReinhold and others added 2 commits December 2, 2022 10:54

fix Description code blocks, add docsStyles parameter to SBUI

29587c9

Merge branch 'next' into tom/sb-996-resolve-css-issue-with-introduction

f02fdb7

JReinhold merged commit e06b1f4 into next Dec 2, 2022

JReinhold deleted the tom/sb-996-resolve-css-issue-with-introduction branch December 2, 2022 13:46

viveleroi mentioned this pull request Jan 13, 2023

[Bug]: Storybook docs CSS cascading into our components #20600

Closed

JReinhold mentioned this pull request Feb 17, 2023

Addon-docs: Fix style bleeding #21150

Merged

5 tasks

anechol mentioned this pull request Dec 16, 2024

docs: updates Storybook font styles to match Pine Kajabi/pine#354

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: using targetted styles, not components to style MDX #19958

Docs: using targetted styles, not components to style MDX #19958

tmeasday commented Nov 25, 2022 •

edited

Loading

JReinhold commented Nov 30, 2022

JReinhold commented Nov 30, 2022

JReinhold Nov 30, 2022

JReinhold Nov 30, 2022

tmeasday Dec 1, 2022

JReinhold Dec 1, 2022

JReinhold Feb 17, 2023

JReinhold Nov 30, 2022

tmeasday commented Dec 1, 2022

JReinhold commented Dec 1, 2022

tmeasday left a comment

tmeasday commented Dec 1, 2022

JReinhold commented Dec 1, 2022

tmeasday commented Dec 1, 2022

		@@ -40,9 +63,374 @@ export const Subtitle = styled.h2(withReset, ({ theme }) => ({
		color: transparentize(0.25, theme.color.defaultText),
		}));

Docs: using targetted styles, not components to style MDX #19958

Docs: using targetted styles, not components to style MDX #19958

Conversation

tmeasday commented Nov 25, 2022 • edited Loading

What I did

Other options

Other notes

JReinhold commented Nov 30, 2022

JReinhold commented Nov 30, 2022

JReinhold Nov 30, 2022

Choose a reason for hiding this comment

JReinhold Nov 30, 2022

Choose a reason for hiding this comment

tmeasday Dec 1, 2022

Choose a reason for hiding this comment

JReinhold Dec 1, 2022

Choose a reason for hiding this comment

JReinhold Feb 17, 2023

Choose a reason for hiding this comment

JReinhold Nov 30, 2022

Choose a reason for hiding this comment

tmeasday commented Dec 1, 2022

JReinhold commented Dec 1, 2022

tmeasday left a comment

Choose a reason for hiding this comment

tmeasday commented Dec 1, 2022

JReinhold commented Dec 1, 2022

tmeasday commented Dec 1, 2022

tmeasday commented Nov 25, 2022 •

edited

Loading