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

Component Boolean Arguments #407

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@hakubo
Copy link

hakubo commented Nov 30, 2018

Rendered

@jessica-jordan jessica-jordan referenced this pull request Dec 3, 2018

Merged

The Ember Times - Issue No. 76 #3714

7 of 13 tasks complete

@amyrlam amyrlam referenced this pull request Dec 10, 2018

Merged

The Ember Times - Issue No. 77 #3722

10 of 14 tasks complete

e.g.
```hbs
<Button @secondary />

This comment has been minimized.

@cibernox

cibernox Dec 17, 2018

Contributor

One possible drawback or unexpected consequence of this is that NOT passing @secondary doesn't make the property false but undefined. While it makes sense, if feels weird that instead of the usual true/false dichotomy we're dealing with true/undefined

@cibernox

This comment has been minimized.

Copy link
Contributor

cibernox commented Dec 17, 2018

I'm not convinced if this small improvement is worth introducing another syntax that is:

  • yet another trick on the book
  • yet another thing to for the template parser to understand and tokenize.

Since I don't find the @prop={{true}} particularly annoying I don't see that much value in this syntax just to save a few keystrokes.

It's worth noticing that @prop=false is, as of today, linted as an error by ember-template-lint by default, which is installed by default by every ember app. People would have to intentionally remove ember-cli-template-lint or intentionally disable that rule (that is enabled by default) to be able to make such mistake.

@hakubo

This comment has been minimized.

Copy link

hakubo commented Dec 17, 2018

The main intention of this RFC is not to save few keystrokes but rather bring unison in how arguments work with how attribute works.
If one learns about that an HTML button can be invoked like <button disabled> I think this knowledge should be sufficient to know how to pass a boolean argument to an Ember component. This should ease the learning of Ember templates for newcomers that know just HTML.

ember-template-lint does say that @prop=true is an error with this message Attribute @prop should be either quoted or wrapped in mustaches but it does not lint against @prop so there would not be any unnecessary churn on users when this is introduced.

@sdhull

This comment has been minimized.

Copy link

sdhull commented Dec 17, 2018

It's possible that I'm thinking of this incorrectly, but in my brain @prop={{true}} is an argument to an angle-bracket component, so if I saw @prop (with no value), I would anticipate that to be undefined (or maybe null?) in my component, but definitely not true.

I understand the desire to make it function more like HTML however I think in this case it would be more confusing that passing an argument without a value somehow defaults the value to true.

As an aside, I think that in HTML, valueless attributes that affect rendering or behavior do so by way of either css selectors (button[disabled]) or equivalent js (thing.attributes.hasOwnProperty('disabled')), not by somehow setting that attribute to true internally.

For example, after adding disabled attribute to the button on this comment form:

button.attributes.hasOwnProperty('disabled')
true
button.attributes.disabled
disabled=""
button.getAttribute('disabled')
""

[Suggestion] So I guess what I think would be cool would be if this didn't set the value to true but somehow modified <div ...attributes> such that when invoking <MyComponent @class="my-component" @disabled>, the div would be rendered like <div class="my-component" disabled>. Then explaining that every attribute @attr is assigned as a key in attributes, and destructuring in the view iterates over attributes' own properties, that all makes sense.

tl;dr defaulting argument values to true is weird imo.

hakubo added some commits Dec 17, 2018

@cibernox

This comment has been minimized.

Copy link
Contributor

cibernox commented Dec 17, 2018

@hakubo I don't see bringing unison between attributes and arguments like a very desirable goal.
In fact, valueless attributes are something I don't particularly like about html. And in several cases it's not even accurate to call then attributes because they are properties, as @sdhull pointed out, although it is true that both are forwarded by ...attributes.

Also at first glance this syntax reminds me a bit more more of positional arguments (which are not a thing in angle-bracket components, and IDK if they will ever be) than of valueless attributes.

@barelyknown

This comment has been minimized.

Copy link

barelyknown commented Dec 30, 2018

I agree with both @cibernox and @sdhull.

First, I don't think that the current API (<FooBar @baz={{true}}>) is confusing or verbose.

Second, I would expect that <FooBar @baz>, if supported (which I'd oppose), would do something like <div class="foo-bar" baz>, and though that would be more in line with my personal expectations, I'd rather it not be supported either given the limited gains and the movement towards tagless components.

While I don't think it'd be that big of a deal if it was supported, I think we should resist any new complexity to the API unless there's a clear win.

@hakubo

This comment has been minimized.

Copy link

hakubo commented Jan 3, 2019

@sdhull
"It's possible that I'm thinking of this incorrectly, but in my brain @prop={{true}} is an argument to an angle-bracket component, so if I saw @prop (with no value), I would anticipate that to be undefined (or maybe null?) in my component, but definitely not true.

I understand the desire to make it function more like HTML however I think in this case it would be more confusing that passing an argument without a value somehow defaults the value to true."

You might be coming from your experience with Ember templates and Angle Brackets as you know them currently. I strongly believe that for new comers, people that come from HTML, React, Vue, Polymer etc it would be natural to think that @prop inside <FooBar @prop /> would be 'truthy'

@cibernox
Sorry for using a wrong term 'attributes'. Let's call them properties. Does that change anything in regard to this RFC?

"Also at first glance this syntax reminds me a bit more more of positional arguments (which are not a thing in angle-bracket components, and IDK if they will ever be) than of valueless attributes."

It does because you know they existed. Again for new comers that would not be an issue. What will be an issue is a realization that they cannot use knowledge that they already posses about passing properties to HTML elements <button disabled> works so <FooBar @baz /> should too.

@barelyknown
"First, I don't think that the current API (<FooBar @baz={{true}}>) is confusing or verbose." - I don't think it's confusing as well. I actually think it is very clear. It is a bit of verbose as the same could be achived with <FooBar @baz />

"Second, I would expect that <FooBar @baz>, if supported (which I'd oppose), would do something like

, and though that would be more in line with my personal expectations, I'd rather it not be supported either given the limited gains and the movement towards tagless components.

What you're describing here IS possible but you do that with components attributes/properties not arguments. e.g. <FooBar baz /> will render as <div class="foo-bar" baz>.


Ember started to position itself (as I get it) as a framework for people that do not have to know much about JavaScript, packaging, building an app etc. Framework that can be used by anyone that know HTML and for that reason I think it make sense for it to adhere to how 'normal' DOM attributes work.

@hakubo

This comment has been minimized.

Copy link

hakubo commented Jan 3, 2019

@sdhull
The alternative approach could be to introduce hasArgument method on component and a helper
so one could do
this.hasArgument('secondary')
and

<button class={{if has-argument(@secondary) "is-secondary"}}>
  Button
</button>

Similar to what is described here: https://developers.google.com/web/fundamentals/web-components/customelements

but that feels clunky IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment