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

Avoid global styles to break components #398

Closed
Grsmto opened this issue Oct 10, 2018 · 9 comments
Closed

Avoid global styles to break components #398

Grsmto opened this issue Oct 10, 2018 · 9 comments

Comments

@Grsmto
Copy link

Grsmto commented Oct 10, 2018

Bug Report

Describe the bug
In docz-theme-default some global styles are applied which are reaching components and breaking the styles on some scenarios:

*, *:before, *:after {
    box-sizing: border-box;
  }

All my components will receive box-sizing: border-box;.

These global rules could be easily avoided by using the box-sizing properties on the necessary elements directly.

A clear and concise description of what the bug is.
docz-theme-default global styles are leaking to consumer components.

To Reproduce
I guess this is self explanatory.

Expected behavior
Styles from docz-theme-default should not impact on consumer components.

I can provide a PR for this if maintainers think this is a valid issue.

@kylealwyn
Copy link

Also experiencing this on the dark theme and the global font color leaking into the live preview:

screen shot 2018-10-16 at 10 42 55 am

@jedrichards
Copy link

jedrichards commented Oct 29, 2018

Agreed ... since this lib doesn't make any attempt to isolate styles between the component previews and the outer theme then the very least that should happen is that global styles aren't used in the theme.

The theme additionally includes normalize.css which can also have a global effect on component styles. It essentially forces you to use normalize.css in your apps too, since otherwise there's no guarantee components will render consistently in docz vs. your app. It's a shame because normalize is pretty bloated, and no longer really appropriate as a modern css reset.

Much better to make the theme CSS more robust and isolated, and reset theme elements individually as needed.

@mergebandit
Copy link

While this is one example, for non-trivial implementations / themes I think it could be a common problem in the wild.

Storybook mitigates this by wrapping each component in an iframe. We're working on a Storybook to DocZ bridge right now, and one of the things we're exploring is wrapping our components in either an iframe or potentially using shadow dom (yay component heights being respected!) to encapsulate each of the story components.

@transitive-bullshit
Copy link
Contributor

Agreed w/ @mergebandit. The storybook-inspired solution to this would be wrapping the MDX React subtree for each page in an iframe that is wholly isolated from the global page.

What @Grsmto is pointing to in terms of CSS isolation is an important point. imho it's related to a larger issue of isolating the host / theme page from any specific page's contents. Since those contents can contain MDX and therefore can transitively include any JavaScript, there's no isolation guarantees for styles, HTML manipulation, or global JavaScript pollution. Practically, the most likely area to manifest issues would be from CSS pollution.

I think a general look into how to properly isolate host / theme styles & code from hosted / mdx styles & code would be great, though at the same time, I'm hesitant to introduce the extra complexity that an iframe layer would necessitate.

@jedrichards
Copy link

Using all: initial, possibly with help from a postcss plugin could be a practical way to achieve an element level CSS reset.

https://developer.mozilla.org/en-US/docs/Web/CSS/initial
https://github.com/maximkoretskiy/postcss-autoreset

@pedronauck
Copy link
Member

Fixed on the new release v0.12.10

@jedrichards
Copy link

jedrichards commented Nov 16, 2018

@pedronauck Would you mind clarifying what's been changed/fixed related to this issue in the latest release? I had a look through and couldn't find anything mentioned.

@pedronauck
Copy link
Member

I removed normalize.css from theme default and the classes and styles applied to Playground are fixed too!

@kachkaev
Copy link
Contributor

kachkaev commented Jan 11, 2019

Looks like this bug still occurs in the latest version. I just cloned the repo and launched the basic example with just the following added lines to packages/docz-theme-default/src/styles/global.ts:

* {
  box-sizing: border-box;
}

Here's the result:

screenshot 2019-01-11 at 21 32 10

(notice gaps next to the resizer)


UPD: 💊 #578

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

No branches or pull requests

7 participants