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

Should class and style should be included in $attrs? #6144

Closed
mrcsmcln opened this issue Jul 17, 2017 · 12 comments
Closed

Should class and style should be included in $attrs? #6144

mrcsmcln opened this issue Jul 17, 2017 · 12 comments

Comments

@mrcsmcln
Copy link

mrcsmcln commented Jul 17, 2017

What problem does this feature solve?

What's the rationale for excluding class and style bindings from $attrs? How else would you bind these attributes to wrapped elements?

What does the proposed API look like?

Consider this field component:

<template>
  <div class="field">
    <input v-bind="$attrs" class="field__input">
  </div>
</template>

<script>
  export default {
    name: 'field'
  }
</script>

Say I'd like to bind the field__input_error class to the <input> element when hasError is true. I'd expect to be able to do it like this:

<field :class="{ field__input_error: hasError }"></field>

But if you wanted to go with the current behavior, you could do this instead:

<template>
  <div v-bind="divAttrs" class="field">
    <input v-bind="inputAttrs" class="field__input">
  </div>
</template>

<script>
  export default {
    name: 'field',
    computed: {
      divAttrs() {
        return this.$attrs.class
      },
      inputAttrs() {
        const { _, ...attrs } = this.$attrs

        return attrs
      }
    }
  }
</script>

I would suggest that we don't overwrite existing class and style attributes as is currently done with other attributes. Also, inheritAttrs would ideally prevent class and style attributes from being passed to the root element when set to false.

@yyx990803
Copy link
Member

yyx990803 commented Jul 19, 2017

class and style have special merge behaviors, and typically they are used for explicitly styling the root element of the child component. Implicitly passing them in $attrs might result in unexpected layout results due to the nesting which is not obvious to the user. If you want your component to expose a way to style the non-root element, you can exposing dedicated props for that purpose, e.g. targetClass and targetStyle. But in most cases, I'd suggest only exposing data-driven props and manage the styling inside the component itself.

@mrcsmcln
Copy link
Author

Implicitly passing them in $attrs might result in unexpected layout results due to the nesting which is not obvious to the user.

Couldn't the same be said about other attributes as well (e.g., id)? While the nesting may not be obvious to the user, it definitely wasn't obvious to me that class and style aren't handled the same way as other attributes, so that alone caused unexpected layout results. When I'm using a wrapper component, I would expect that all regular HTML attributes are passed to the wrapped element.

@yyx990803
Copy link
Member

yyx990803 commented Jul 19, 2017

The reasoning here is that $attrs are meant for passing down generic, non-styling related attributes. class and style are almost always styling related, this is actually noted in the docs (we should probably make it more prominent). When I am talking about expected result to the user, I am talking about user of the component that you provided, not you as the user of the framework.

When authoring components, I don't think it's a good idea to encourage the user to directly style the component via classes or inline styles. As I said, prefer pure data props and manage the styling inside the component. your example could just be:

<field :has-error="hasError"></field>

Now the user of the component don't even need to know about the field__input_error class.

@jcupps
Copy link

jcupps commented Apr 17, 2018

I agree with @mrcsmcln that while avoiding unexpected results is a good rationale for the behavior, excluding certain arbitrary attributes is less transparent/consistent and simply causes unexpected results in a different way (though the clear documentation is appreciated). In this case, I argue that consistency and clarity are more important.

Additionally, it appears there is currently no way to apply class and style bindings to a child element instead of the root without using custom props, while the existing behavior would still be achievable if those two attributes were not excluded as the OP suggests by binding this.$attrs selectively (which would be more verbose but also more explicit/transparent).

For me, this is compounded by the inability to have more than one root element, which forces us to use wrapper elements (e.g. for HOCs) to which we may not want to apply style and class bindings.

Side note: the documentation seems to imply that class should be included in $this.attrs: https://vuejs.org/v2/guide/components-props.html#Disabling-Attribute-Inheritance

@TrendyTim
Copy link

I ran into this trying i have a text input component that needed to be in a div (making a input with font awesome in a span in the textbox) , 99% of the time styling should be applied to the input not the div, would be nice if we could get an inheritStyle option (which would also include class) then we can say

<div>
<input class="$class + ' someOtherClass'" />
</div>

or even some kind of nicer way to merge the class/style data.

I had to deal with it by adding a specific property for passing styling to the input, which is annoying and non intutitve.

While generally i can agree with not exposing styling of inner components like that, sometimes for simple ones it makes sense (and even some complex ones there may be a default item to apply the style to)

Other times as @jcupps mentioned vue forces us to abuse div tags because it thinks there might be more than 1 root element (even though i KNOW it never will) and we cant use template tags to get around it.

As a contrived but simple example you cant do

<input type=text v-if="!multiline">
<textarea  v-else>

because the node types are different and vue doesn't like an else on a different node type. so you have to

<div v-if="!multiline">
<input type=text >
</div>
<div v-else>
<textarea >
</div>

And then class= gets applied to the div regardless of inheritAttrs even though its only there for vue and if anything it gets in the way of the html (but thats a different topic).

@ianef
Copy link

ianef commented Apr 25, 2018

This has been causing me grief too, I'm having to use props with un-obvious names to pass class attributes down to my reusable components.

yyx990803 you stated "When authoring components, I don't think it's a good idea to encourage the user to directly style the component via classes or inline styles." But in my mind the whole point of making reusable components that are flexible is that they can be styled by the container, thus making them easy to use without refactoring every time you want to change say the background colour or text size.

Just the name of the property 'inheritAttrs' should imply exactly that, maybe it should be renamed 'inheritMostAttrs' that way new users would get a clue!

Having to write computed properties in 90% of my components to deal with applying classes and styles to the inner elements is quite frankly very annoying and wastes time when you forget.

Personally I think the default behaviour should be changed to exclude all attributes if the flag is false, however this would now be a breaking change so how about a related property 'inheritStylingInAttrs' which would default to false and not include them, setting it to true would force the behaviour we are all looking for and would have expected in the first place.

@MightyPork
Copy link

I've just stumbled upon this design flaw myself.

I wanted to create an accessible icon component:

<v-icon class="fa-comment pr-1" alt="Add Comment" />

which would become:

<span class="sr-only">Add Comment</span>
<i aria-hidden="true" class="fa-comment pr-1" title="Add Comment" />

First obstacle is I can't have multiple root elements, okay, I wrap it in span. But then I discovered class is excluded from inheritAttrs with no way to suppress it. I had to rename the attribute to x-class.

My working but ugly component is now as follows:

<template>
  <span>
    <i :class="xClass" aria-hidden="true" :title="tooltip"></i><!--
    --><span class="sr-only" v-html="alt"></span>
  </span>
</template>

<script>
export default {
  inheritAttrs: false,
  props: {
    xClass: String,
    alt: String,
    srOnly: Boolean
  },
  data: function() {
    return {
      tooltip: this.srOnly ? null : this.alt
    };
  }
}
</script>

Used like <v-icon x-class="fa-trash" alt="Delete column" />.

It works, but it's needlessly complicated. I could use v-bind="$attrs" on the <i> and use regular class attribute, weren't it for this problem. That would also make jumping to class definition from the component instance work in my editor, now it doesn't even know x-class is a class attribute.

(yes, I could put the sr-only span inside the <i> and not make it aria-hidden, but this is just one example)

@dudo
Copy link

dudo commented Aug 25, 2018

Can this be reopened? If we can have the freedom to attach $attrs to the element we want, why can't we be given the same freedom for $styles?

@tinyfly
Copy link

tinyfly commented Dec 17, 2018

By the way, the guide docs about component props have class in the disabling attribute inheritance example. I spent a lot of time trying to figure out why class wasn't coming through until I looked up the api docs on $attrs.

@MattiasMartens
Copy link

If components are meant to be considered by the user as fancy single HTML nodes that can be modified as normal HTML can, then the inheritAttrs option runs counter to that paradigm.

If, however, components are meant to be functional—they serve a particular purpose for the parent, however that purpose can most effectively be achieved—then it makes sense to give the component designer control over how HTML modifications from the parent are implemented.

What I like about Vue, generally, is how comfortably the beginner and advanced modes tend to coexist. There is a simple language that covers the majority of use cases, and when a new constraint causes that language to break down, a developer can quickly find the expanded toolkit that allows them to dig as many layers down as they need to in order to thoroughly solve the problem.

For simple use cases, it makes sense to think of components as fancy HTML nodes. For more advanced cases, it makes sense to think of them more abstractly, as managers of a branch of the DOM tree. This is especially true when, for whatever reason, the root element of the component can't be the same as the root presentational element.

Generally, I would expect an option like inheritAttrs to be a bridge between the simple and advanced concepts of what a component is. In this case, though, it falls flat because it does not provide a uniform interface to that higher level of abstraction.

class and style are standard HTML attributes. There is no conceptual reason to treat them differently than any other standard HTML attributes. If passing them to something other than the root HTML node of a component is surprising and unexpected, then so is passing any other standard HTML element in this way. By far the most surprising outcome is to pass down some standard HTML attributes and not others.

@ianef
Copy link

ianef commented Mar 13, 2019

I totally agree, well phrased Mattias.

Sadly it doesn't seem to be getting us anywhere. The Vue elite seem to be ignoring this conversation.

@yyx990803
Copy link
Member

yyx990803 commented Mar 14, 2019

In 3.0 class and style will be included in $attrs. Keep an eye out for upcoming RFCs!

@vuejs vuejs locked as resolved and limited conversation to collaborators Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants