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

Scoped styles for child components #573

Closed
BernieCr opened this issue Aug 7, 2019 · 21 comments
Closed

Scoped styles for child components #573

BernieCr opened this issue Aug 7, 2019 · 21 comments
Milestone

Comments

@BernieCr
Copy link
Contributor

BernieCr commented Aug 7, 2019

I really love working with this library, the only thing that bugs me is that currently there is no simple way to pass scoped styles to child components:

<div>
   <h1 className="header">Login</h1>
   <CustomButton className="login-button">Submit</Button>
   <CustomButton>Cancel</Button>
</div>
.header {
   font-size: 50px;
}
.login-button { /* won't be applied currently */
   color: red;
}

This issue had already been discussed several times (#273, #197, #121). So I thought the current behavior (with the :global() workaround) was due a technical limitation. However, after digging into the code a bit, I discovered that it's actually quite easy to apply the jsx-* scoped class name to child React components as well.

I already created a PR for this. My change adds the jsx-* class to all child React components where a custom className is passed. In the example above, that apply to the "Submit" button,
but not the "Cancel" button. This leads to the following code being generated:

<div class="jsx-4088848069">
   <h1 class="jsx-4088848069 header">Login</h1>
   <button type="button" class="button button--secondary jsx-4088848069 login-button">Submit</button>
   <button type="button" class="button button--secondary">Cancel</button>
</div>

As you can see, the button with our custom class "login-button" gets the jsx-* class, the other one doesn't. Works like a charm, the scoped style for .login-button is applied correctly. It also doesn't mess with the existing classes of our child components ("button button--secondary" in this example).

So are there any reasons I'm currently missing why this bevahior isn't possible or advisable? I'd really like this feature to be included.

@giuseppeg
Copy link
Collaborator

Quoting myself:

This change is going to be a breaking one as currently users don't expect to get a scoped class. I guess I will keep the PR open until we decide to cut a major release and evaluate if it makes sense to introduce this change

@giuseppeg giuseppeg added this to the v4 milestone Aug 8, 2019
@pushkine
Copy link

🎉 Any idea when this is getting released ? or is there a way to use this with nextjs without editing package-lock.json ( as it is a nested dependency ) ?

This change is going to be a breaking one

Could make it opt-in ?

@haf
Copy link

haf commented Jan 15, 2020

@giuseppeg Can you consider hiding this functionality behind a feature flag? That way one can use this package like one would normally use css.

The issue addressed here is compounded by not having anything but element selectors apply when css.resolve is used.

Further compounded if you have this case;

  • Component A, styles A loaded initially, has a className='link'
  • Component B, styles not loaded initially, also targets .link, but globally since it uses composition of React components and therefore is forced to use :global(.link)
  • As component B lazy-loads, styles break for A, not caught in tests, because loading B and then A is not the tested/default flow.

CSS stands for "Cascading Style Sheets" and should be defaulting to that behaviour.

@bernhardc How do I use your pacakge instead of zeit's?

@bySabi
Copy link

bySabi commented Jan 15, 2020

@haf I make a PR with @bernhardc changes and made it opt-in: #605 just as it was discussed to on the original @bernhardc PR: #574

Although I myself conclude that it is not possible, nor desirable, to apply styles to child components just as the styles are applied to nested html tags.

@haf
Copy link

haf commented Jan 15, 2020

I think my point can be summarised as:

  • It's alright to cascade down, because you're working on your component and its children
  • It's not alright to cascade up (with global) from a component
  • I think this library should enforce the two above principles

@bySabi
Copy link

bySabi commented Jan 15, 2020

I'm with you @haf but ...

  • cascade down stop when a child component is found. Go beyond that will be break React components encapsulation.
  • styled-jsx is just a tool that mimick CSS selectors, actually stylis do this, and inject className into "safe" boundaries, html tags not React "tags" (components)

With the @bernhardc PR, and my PR, styled-jsx could inject a className to a child component like this

<MyComponent className="jsx-inyected_className">
   <AChildComponent className="jsx-inyected_className">
...


const AChildComponent = ({ className }) => (
   <div className={className}>
      <p>Testing child <span className="other-color">Component</span></p>
  </div>
)

But this is not really useful cause even with this PR you will never reach AChildComponent nested tags or classes like: p, span, .other-color using selectors provide by styled-jsx. In the end you will need to use :global

You can test by your self cloning my initial proposal repo that include this PR: https://github.com/bySabi/styled-jsx-repo

@haf
Copy link

haf commented Jan 15, 2020

@bySabi I see, I thought I'd be able to reach .other-color from the parent in your example. My use-case is a complex menu where I want each thing as a component:

  • header
  • nav
  • hamburger button
  • search button
  • logo
  • expanded menu in small screen mode
  • wide-screen mode menu styles
  • the menu items, including dividers and constant items (fetched via GraphQL from a Headless CMS)
  • the menu also works for search - so the animations and CSS transitions both for all of the above (everything is animated), as well as the search input field styles and the state changes needed for show either an expanded, or hamburger, or small-screen expanded, or wide-screen serach or small-screen search, as well as the search results as images/tabular data

Almost everything inside this menu should inherit my CSS from the parent, and since there's such an abundance of things inside children, I'd have to create searchButtonExpandedStyles, searchButtonExpandedClassName, searchButtonSmallClassName, searchButtonSmallStyles; and that's just two out of the three widths we're designing for, and a single small component. But even with this; it's a leaky abstraction, since I don't want to style the implementation — the element types in vanilla HTML — but I want to style the boxes labelled with intent through class attributes.

A lot of the styles also need to know whether the menu is expanded, which is done by having a selector matching the parent's .menu-open class name, then styling the menu items, buttons and so on.

I don't fully understand why I should need React component encapsulation to the extent you're suggesting. As long as I use specificity as it's designed, its effect should be that if I qualify a child component's selector more from the parent, it applies, otherwise it doesn't. And then since I'm using styled components, at least I don't have to think about e.g. the styles of an input leaking into a global scope and polluting the whole site, like I have to if I use stylus/SCSS.

I mean; the alternative is just to go back to "vanilla SCSS". Even just having a unique number generated at my point of choosing, and having that (class name) number prefixed before every selector that gets loaded by my overly competent menu would be enough for me.

Don't other people do apps/web sites like me? 🤷‍♂️

@bySabi
Copy link

bySabi commented Jan 16, 2020

@bySabi I see, I thought I'd be able to reach .other-color from the parent in your example. My use-case is a complex menu where I want each thing as a component:

No, you can't unless you use :global

I don't fully understand why I should need React component encapsulation to the extent you're suggesting.

A well designed tool cannot break the encapsulation of the component because it cannot be assumed how it will be rendered. A React component is not an HTML snippet but a javascript code.

As long as I use specificity as it's designed, its effect should be that if I qualify a child component's selector more from the parent,

This is still valid but applies when the application has been rendered in the browser.

And then since I'm using styled components, at least I don't have to think about e.g. the styles of an input leaking into a global scope and polluting the whole site

I don't know for sure how this is implemented with the styled components but at the end of the day it can only be implemented by injecting inline styles or with generated unique classNames.

React is basically Javascript that inserts Nodes elements in the DOM using the browser API but does not know anything about CSS, except inline styles. So it doesn't handle concepts like cascade, specificity or inheritance. This is a browser thing.

At the end of the day you will have to use a strategy to stylize the components in insolation with already existing JS tools (styled, styled-jsx, ... ) or with utility classes CSS approach like TailwindCSS

or use global CSS files using vanilla CSS or preferably with custom variables and PostCSS or SCSS or ...

In my personal opinion it is better to use global CSS together with a methodology or architecture that helps you avoid the common problems inherent to CSS. In my case I use

C3CSS which in turn is based on Enduring CSS and BEM
You can read here about them

With a good layout system like: https://every-layout.dev/

@haf
Copy link

haf commented Jan 16, 2020

A well designed tool cannot break the encapsulation of the component because it cannot be assumed how it will be rendered.
React is basically Javascript that inserts Nodes elements in the DOM using the browser API but does not know anything about CSS, except inline styles. So it doesn't handle concepts like cascade, specificity or inheritance.

From what I can gather this is a lib that traverses the AST at compile-time and matches selectors to elements/discovers styles. Since the AST contains logic that cannot be evaluated at compile-time, classes cannot be assigned directly in all cases. So all selectors are hoised into a stylesheet-registry and then applied at render-time, logically every time render happens. Rendered react components are a tree, so that tree can be traversed in order to cascade selectors.

@bySabi
Copy link

bySabi commented Jan 16, 2020

But if you skip the limits of the component where is the limit of the parser?
This was discussed in the issues some time ago and someone proposed an option to indicate the maximum level of depth.
And I think that from there it came up to introduce :global but I am not sure that it is exactly like that.

@haf
Copy link

haf commented Jan 16, 2020

Well, I don't think there should be a depth limit; it should work like in the browser's cascades. Hopefully you don't have react component trees thousands of items deep.

@bySabi
Copy link

bySabi commented Jan 16, 2020

Well you already have the ingredients, need and dissatisfaction, for the next big thing on CSS/React tooling
It will be a pleasure to try it when you have an alpha release :-)

@decimoseptimo
Copy link

I always thought It'd be to cool to define "from this component and downwards, inherit." I tried to use css.resolve, too much hassle.

I decided to use global at the top whenever I needed inheritance, so far so good.

@mikestopcontinues
Copy link

Has there been any motion on this? I'm considering using styled-jsx for my next project, but this is a real dealbreaker. A child component shouldn't handle its own positioning. That's definitely for the parent to handle. What's the justification for disallowing className support on children? There's no danger of mis-application as far as I can see...?

@BernieCr
Copy link
Contributor Author

BernieCr commented Dec 13, 2020

I'm not currently using the library, so I stopped pushing this issue forward.
But there is still my original PR #574, and someone else built on top of that in #605 and put the feature behind a flag. The maintainers' original concern was that this feature would be a breaking change, so making it opt-in like in the PR should alleviate their concerns.
If anyone was willing to take up the PR again, I think there should be a good chance to get this feature merged.

@decimoseptimo
Copy link

decimoseptimo commented Jan 5, 2021

@giuseppeg this feature is important from a DX standpoint. Too verbose to create a new jsx-??? with css.resolve when we could reuse the one generated by <style jsx> . Opt-in' should do fine.

@theoludwig
Copy link
Contributor

Agreed, I would love to have this feature in styled-jsx.

Even if it's a "breaking change", why are we not doing one ? Maybe it's time to release a new major version.

@bersoriano
Copy link

Any new ways about handling this problem in 2021?

@rmariuzzo
Copy link

Just noticed this issue while giving this library a chance over CSS Modules and SC. That's really something that we need.

@huozhi
Copy link
Member

huozhi commented Sep 5, 2021

There're few discussion here that enabling this feature by default is bit unideal, unless we can have other elegent solution to solve these problems mentioned there:
#574 (comment)
#574 (comment)

@nitz-iron
Copy link

+1 for this feature, I reverted to css module for those cases.

@huozhi huozhi closed this as completed Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests