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

Support classes on nested components #2888

Closed

Conversation

nikku
Copy link
Contributor

@nikku nikku commented May 27, 2019

This adds class handling logic to InlineComponent:

  • Marks classes on components as used
  • Passes the scoped class string to the component instance

See my comment for a concise description of the behavior addressed.

Closes #2870

@nikku nikku force-pushed the 2870-keep-class-nested-components branch from 539c0a8 to 04d6604 Compare May 27, 2019 20:41
@nikku nikku marked this pull request as ready for review May 27, 2019 20:42
@nikku nikku force-pushed the 2870-keep-class-nested-components branch 3 times, most recently from ba3677a to 5214c75 Compare May 28, 2019 18:33
@nikku nikku force-pushed the 2870-keep-class-nested-components branch from 5214c75 to cd43340 Compare May 28, 2019 18:35
@nikku nikku changed the title keep class declarations passed to nested components Support classes on nested components May 28, 2019
@Conduitry
Copy link
Member

We do want to support something providing this sort of functionality, but giving special treatment to the class prop doesn't feel like the correct solution. I'm not sure yet what mechanism I would like, though.

@nikku
Copy link
Contributor Author

nikku commented May 30, 2019

Thanks for the feedback.

I'll keep this fully functional PR open for you as an inspiration then and will publish and use my fork in the mean time.

@Narigo
Copy link

Narigo commented Jun 26, 2019

After looking through the open issues and trying out the various workarounds, I see this PR as the only possibility to override css in a child from a parent in a safe way: If I try to pass a custom css class from a parent without using :global, the class will be removed by the optimization. But using :global means I need to know implementation details from the child and I might accidentally overwrite some css of a grand child, if the class does not happen to be a direct child of something I have (and lets me use the > :global(...) operator).

I think it's best to let the child decide what kind of things you should be able to override. The issue we face is that the optimizer removes classes from the css, when it's not used. So we need to provide a way to mark the class as used.

I can't really think of good alternatives, but I'm not using Svelte for long. @Conduitry do you have alternative approaches you might want to share for discussion? :)

The only alternatives I can think of:

  1. Check all class tags, irrelevant of html or svelte components and keep their declaration. Children can use export { customClass as class } to retrieve it.
  2. Have something like a custom add:class that lets the child component retrieve that somehow and mark this as "class is used". Have the child be able to access it somehow.

I think No. 2 would be quite a bit more involved and I do not really see great benefits to it. If someone already uses class as a prop, this would not even break it - it's actually just an added feature to not loose the hashes, or am I wrong here?

@sheijne
Copy link

sheijne commented Nov 2, 2019

@Conduitry

We do want to support something providing this sort of functionality, but giving special treatment to the class prop doesn't feel like the correct solution. I'm not sure yet what mechanism I would like, though.

How about passing all undeclared props down as attributes?

  • It would give the user of the component the ability to gain full control over the attributes on the component's root elements.
  • When an attribute exists on a root element as well as the implementation of the component, the value can be overwritten by the implementation's value.
  • To overwrite the default behavior developer of the component can hijack attributes by declaring them as props.
  • 'Transparent' components would be.. Transparent.

I see two problems here:

  1. The first being scoping of classes, we'd potentially create a world of scope spaghetti. As a solution a separate scope could be created as a bridge between 2 components.
  2. The class attribute would still be a preferred special case, since it should probably be merged rather than replaced. Giving developers the ability to pass a merging strategy along with the attribute could be a possible solution.

Let's paint that picture..

MyComponent.svelte:

<script>
  export let bar = true;
  export let contenteditable = false;

  // Some kind of merge implementation, perhaps following the reducer pattern since we'll be dealing with multiple values from potentially multiple parent components.
  const merge = (accumulator, currentValue) =>
    currentValue ? `${accumulator} ${currentValue}` : accumulator;
</script>

<Component class|merge="foo" class:bar|merge {contenteditable}/>
<Component class="magic-component"/>

Component.svelte:

<div class="component"/>

Output:

<div class="component svelte-CHILD_HASH foo bar svelte-PARENT_HASH"></div>
<div class="magic-component svelte-PARENT_HASH"></div>

Something like that.

Writing this example feels a little funny, but it's a start. To make it a bit sweeter simple string concatenation would suffice as a default merge strategy since attributes may only contain string values anyway. DOM props are a bit more complicated I guess..

Most of this is inspired by the way VueJS handles inheritance of attributes. I'd love to see a feature like this, it would also make it a lot easier to extend existing components. Semantically speaking the components that we're writing are still simply wrappers of DOM nodes, which make me feel this is an essential feature.

@Rich-Harris
Copy link
Member

Finally weighing in, because I think we now have a good solution to this problem. For me the key issue is this:

I think it's best to let the child decide what kind of things you should be able to override.

Giving class special treatment is a clever solution, but it's a bit magical, and doesn't completely cede control to the component — class is a blunt instrument which makes it hard for the component to expose control over specific styles at arbitrary depths of its own markup, and which allows the component consumer to change styles that it has no business controlling. It also means that component authors need to expose class as a prop, but it's likely that this would be applied inconsistently, causing confusion in the ecosystem.

I think the right way to solve this issue is with CSS custom properties. It can easily be done today (just have a wrapper element that applies the custom properties using a class or an inline style), and this RFC proposes a quality-of-life improvement that would let you pass custom properties directly to components (by injecting the wrapper element for you).

The RFC is still open for the time being but I anticipate something close to it being what we end up with, so I'll close this PR — thank you

@geirmarius
Copy link

geirmarius commented Nov 16, 2019

Joining the conversation late - but I've recently gotten to try out svelte (and love the idea! ❤️) and have run into this problem as well. The proposed RFC is great, but does not solve where I see the problem lies.

There is a good amount of properties that should mostly be applied from a parent's point of view. We're talking stuff like grid-area in grid layouts, margin and flex in flex layouts. Even properties like position and and the top/right/left/bottom following it in some cases.

It would be tiresome - and bloated - to include a class pass-through for every component or assigning custom properties (from the RFC linked) for all potential properties on every component, just in case it's gonna be used in layouts that requires it. Wrapping them in a wrapper div is certainly an option, but potentially creates 100s or 1000s (long lists, several lists etc.) of new elements in the DOM slowing down low-end devices.

Ideally it'd be as easy as:

<button class="button">
  Like
  <Icon icon="heart" class="button__icon" />
</button>

<style>
  .button {
    display: flex;
    align-items: center;
  }

  .button__icon {
    flex: none;
    margin-left: .5rem;
  }
</style>

Problems I see:

  • Potentially multiple root elements (or none?).
    Can it be applies to every root element?

  • If both parent and child have a rule-set for the same class-name applied to the child, they'll both be applied to the root element of the child, even if the child's rule-set was meant for another element*
    This is because svelte appends a class-name. Could it be solved by svelte prepending a hash to the class-name it self? .my-class-svelte-123xyz

  • I do not have a good solution for class being magical compered to other properties
    Here Vue applies all properties as attributes if they're not defined as a component prop. Is that a solution?

I'd love to hear thoughts on this. Sorry in advance if I've missed something - still new to svelte. 🙏

Edit: For some reason I missed sheijne's comment, but it addresses some of my concerns as well. #2888 (comment)

@pngwn
Copy link
Member

pngwn commented Nov 17, 2019

This is the probably wrong place for this discussion but to answer one of your questions:

I do not have a good solution for class being magical compered to other properties
Here Vue applies all properties as attributes if they're not defined as a component prop. Is that a solution?

The main reason using classes isn't a great solution is that it completely breaks encapsulation in a confusing way, the paren't shouldn't be dictating anything, the component itself should. The parent can pass things and the child can choose to use them or not but that is different: control is still in the hands of the component itself, not an arbitrary parent.

This isn't a solution bvecuse svelte doesn't need a single root node in a component, should they be applied to all top level children? I don't think this would work.

@nikku
Copy link
Contributor Author

nikku commented Nov 17, 2019

The point we're probably missing here: The main rationale for this PR is that, in my hones opinion, Svelte needs a way to support style overrides in an intuitive and close to plain HTML/CSS way. What I regard as intuitive is: Looking at how customizing of styles is being done when applying a typical CSS component framework, and making that possible with Svelte.

One way to customize styles in literally any CSS framework works by overriding default styles via contextual (higher binding) selectors. Those by nature interfere with what the child defined, whether the child wants or not and also in ways the child component author did not anticipate. It is not the question whether you should do that but rather whether you can.

Closing this PR essentially means: Svelte will not offer a generic way to support style customizing via contextual class overrides (as we'd do it in plain HTML). Instead we'll invent something new that is entirely different. If a child component is provided and does not anticipate some contextual usage scenario (style wise) you'd need to copy it or hack around that via :global hacks.

That is a valid choice.

@pngwn
Copy link
Member

pngwn commented Nov 17, 2019

Svelte needs a way to support style overrides in an intuitive and close to plain HTML/CSS way.

Again, this breaks the model, I argue we do not want that. This isn't behaviour we want to encourage so the current situation where it is possible but annoying/ difficult is ideal: it is there as an escape hatch but not encouraged.

It is not the question whether you should do that but rather whether you can.

This is a framework and it comes with certain opinions about how things should be done, this isn't unique to Svelte. And before we can decide whether or not we will allow certain behaviour or encourage it with better ergonomics, we have to have a conversation about whether or not we should be doing things that way. You can't separate the can from the should in an opinionated framework. We want to make building UIs simpler, for sure, but also safer we don't want to add ease of use at the expense of component encapsulation, there has to be a balance

For my point of view, and I've been annoyingly consistent in this for as long as people have been asking for this feature or something like it, style encapsulation is one of the core principles of Svelte's component model and this feature fundamentally breaks that. It would be too easy for people to use this feature and it would definitely get abused removing the style safety that Svelte previously provided.

The RFC is more appropriate because it does not allow a parent to abritrarily control anything below it, that responsibility still relies on the component itself. Just because people have been passing classes round and overriding child styles for years doesn't mean it is a good choice and isn't something we wnat to encourage. Explicit interfaces are preferable, even if it places greater demand on library authors to design both their components and their style interfaces with these things in mind.

@geirmarius
Copy link

margin, flex, position, left, right, top, bottom, width, height, align-self, justify-self among other is CSS properties that should never be modified by the child itself. The parent should always have control of those properties, which is the whole reason I'm asking for this.

Because they affect the outside of the component, this should purely be controlled by the parent hosting them.

These are my opinions, but I stand strongly by them and it's by far the most efficient, performant and simple method of dealing with CSS I've found. Unfortunately CSS is built the way it is, and wasn't necessarily built for what it has grown to, but that doesn't mean we can start ignoring it.

Ideally: Only let a parent control those specific CSS properties, and never let a child use them on the root element.
Of course, that brings a whole other set of problems.

@pngwn
Copy link
Member

pngwn commented Nov 17, 2019

I disagree with this for the simple reason is that a component isn't a visual concept. The component doesn't end where its box-model ends. A component doesn't need any DOM at all, in fact, and what DOM it does have definitely shouldn't be styled from outside.

If you want this control then wrap them in a DOM node that the parent controls. If you want to pass in values then use props and if you want to pass in values from higher up the tree, the new style RFC may be able to help.

@sinnlosername
Copy link

sinnlosername commented Dec 27, 2019

Finally weighing in, because I think we now have a good solution to this problem. For me the key issue is this:

I think it's best to let the child decide what kind of things you should be able to override.
@Rich-Harris

This is a great concept for javascript, but I don't think this will work for css. There are many things you can style using css. It will result in the following two cases:

  1. The component developer exposes ALL possible css properties using the system from RFC 13. This will create lots of unnecessary code & work. Just think of all the css properties a developer might wanna change (color, background-color, height, width, position, left, right, top, bottom, ...). Adding all of them would be not very svelte.
  2. The component developer does not expose all possible css properties. This will upset the developer using the component library because he can't style his page like he wants to do. Also not very svelte.

In most established component libraries from other frameworks (e.g. Material UI) you can also pass certain css properties using component properties (similar to RFC 13), however this is almost never enough. It happens just so often that you need to do some slight modifications in the css so the result fits your expectations.

Svelte has already added a lot of innovation to web development, but I think in this case we should not reinvent the wheel.

@gushogg-blake
Copy link

I've encountered a few scenarios where passing a class down from a parent component feels like the cleanest way of doing it as well. I think this is being rejected on grounds that are too arbitrary, and detract from what to me are the best things about Svelte -- it's fun and easy to use, and lets you write components in a way that's natural and expressive. The problems I see with the --variable RFC are:

  • verbosity for multiple overrides in parent component markup
  • variables and fallbacks in the child component css again seem verbose, and not as flexible as classes
  • strange syntax that subjectively looks hacky to me, but would be a part of the framework
  • you can't use SCSS variables

I disagree with the idea of keeping users "safe" from this feature. Web developers are well aware of the mess you can get into with global CSS, and the action of writing <Child class="foo"/> and <div class={_class}>` (or similar) in the child component is an explicit indication that, while taking advantage of all the greatness of style encapsulation by default, in this case you have decided that you want a very specific and controlled "leak", of one class, from one component instance to one component instance.

The fears of breaking one of Svelte's core tenets seem overblown to me. Style encapsulation by default is great, but that doesn't mean we should contort the framework around it.

I do see the problem of treating "class" specially, but it doesn't seem like a deal-breaking issue -- we could put it behind a compiler flag like componentClasses: true for example, or componentClassName: "className" for people who want to use it but are already using "class" for something else.

@non25
Copy link

non25 commented Aug 10, 2020

Want to use this now?

Existing projects

A more robust and maintainable way to incorporate this patch into your workflow using patch-package:

Updating

When svelte is updated by using npm update, run npx patch-package again.

If it fails due to substantial svelte updates you have two options:

1: Download new patch

2: Make new patch yourself

  • Use patch utility, it guesses where changes should be applied based on the context and in the case of this patch, does it pretty well

    patch --no-backup-if-mismatch node_modules/svelte/compiler.js patches/svelte+<version>.patch
    
  • Generate package-lock.json if it is not there: npm i --package-lock-only

  • Make a new patch: npx patch-package svelte.

  • Delete package-lock.json if you are not using it.

New projects

Download patched sveltejs/template using degit:

degit non25/svelte-template-rollup#nested-component-scoped-class svelte-app

Info

This allows passing classes to child components with svelte-{hash} through the class prop and prevents removing such classes from css.

Child component should define a place where classes will be recieved:

<button class="button {$$restProps.class || ''}"><slot /></button>

class: directive is not implemented.

@PaulMaly
Copy link
Contributor

PaulMaly commented Oct 2, 2020

Too many bones were broken about this issue... @nikku really thank you man for your time and providing great PR!

Unfortunately, with the current style hashing approach, your solution will be the cause of many confusing edge-cases. Even with the simplest uses. ;(

Let's leave behind brackets the esoterical question - is it right or wrong. Actually, the only way to make your solution safe is to change the principle of style isolation and at the same time restrict which selectors allowed. For example, CSS modules can't be applied to tag names, only to classes, and hashing mechanism quite different.

In Svelte, you'll be continually faced with things you completely break 3rd-party components, for no reason whatsoever. The simplest example I could imagine: REPL

This breaks the component even without actual class passing (parent class removed from the output as usual) and your patch will only make it worse, because result styles will be something like:

<div class="foo svelte-child-hash bar svelte-parent-hash">

So, this part - svelte-child-hash bar - will break the nested component's behavior in various, unpredictable manners. The landscape of potential edge-cases is too big to implement this in Svelte and thereby approve its use.

As I said, the only way to make this possible is to completely re-write the css isolation approach and restrict the rules for writing selectors. I'm not sure we should do this.

Thanks a lot to everyone who joined this and similar discussions! Good luck!

@valen214
Copy link

valen214 commented Oct 5, 2020

(sorry last link was wrong)

https://svelte.dev/repl/11d9f53c21e24fa9bfcf7140ac758ac4?version=3.12.1

I like this workaround but it has downsides

@rcline
Copy link

rcline commented Oct 30, 2020

Workaround by using scope-nested global classnames:
#2870

@non25
Copy link

non25 commented Apr 21, 2021

Solution! Is it visible enough?

We can just end this suffering at the cost of scoping for non-class selectors and meet the consequences of class-passing, whatever it might be.

There's a cssModules scoping preprocessor, which allows to:

  • write class="some-class" instead class={css.someClass}
  • use the style tag
  • pass hashed classes to components without div wrappers
  • be safe of inheriting outer site styles because we used some generic .modal class in our widget-app
  • avoid selector weight wars

Try it

Install it:

npm i -D svelte-preprocess-cssmodules svelte-as-markup-preprocessor

Adjust preprocess config in the bundler:

const cssModules = require('svelte-preprocess-cssmodules');
const sveltePreprocess = require('svelte-preprocess');
const { asMarkupPreprocessor } = require('svelte-as-markup-preprocessor');
// ...
preprocess: [
  asMarkupPreprocessor([
    sveltePreprocess()
  ]),
  cssModules()
],
// ...

Use it:

<button class="btn {$$restProps.class || ''}"><slot /></button>
<!-- something else below -->
<div>
  <p class="future-is-bright">The future is bright</p>
  <Button class="margin-top">Click me!</Button>
</div>

<style lang="scss" module>
  .future-is-bright {
    color: bright;
  }

  .margin-top {
    margin-top: 6px;
    // whatever you want
  }
</style>

Preprocessor readme

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

Successfully merging this pull request may close these issues.

Passing class to components