Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

Add support for [attr.name.if]="boolean" syntax #1058

Closed
matanlurey opened this issue Mar 12, 2018 · 17 comments
Closed

Add support for [attr.name.if]="boolean" syntax #1058

matanlurey opened this issue Mar 12, 2018 · 17 comments

Comments

@matanlurey
Copy link
Contributor

Similar to [class.name]="boolean", which adds a class 'name' if boolean is true, and removes class 'name' _if boolean is false (or I think null?).

For example:

<button [attr.if.shiny]="isShiny"></button>

Would output <button shiny></button> if isShiny == true, and remove the 'shiny' attribute if isShiny != true. The reason for this new feature, versus re-using the existing [attr.shiny] syntax, is that it strictly coerces the values to strings, and writes them in the template:

<button [attr.shiny]="isShiny"></button>

... outputs <button shiny="true"></button>.

We talked about trying to check, and if the value is a bool use the new behavior, but that is (a) breaking, and (b) not definitive (it's possible for the value to be Object or bool and require runtime checks). It's probably not worth it.

@matanlurey matanlurey added this to the >5.0.0 milestone Mar 12, 2018
@matanlurey
Copy link
Contributor Author

BTW if anyone has better syntax alternatives, suggest them!

@zoechi
Copy link

zoechi commented Mar 13, 2018

null as expression result is supposed to remove the attribute.
It's a while I used it, but I'm sure this worked already.
Are you requesting a different variant where false removes the attribute?

@akaufmann
Copy link

I think the best way would be to make it a breaking change to be (a) in line with e.g. [class.name]="boolean" and (b) to have the same behavior as other frameworks e.g Vue.js (if that counts) _if_ this could land before the stable release of Angular 5.

<button [attr.shiny]="false"></button>
<!-- <button></button> -->

<button [attr.shiny]="null"></button>
<!-- <button></button> -->

<button [attr.shiny]="myObject"></button>
<!-- <button></button> -->
<!-- Maybe a warn?: [attr.shiny] can only handle Boolean values. myObject is an object, therefore the attribute 'shiny' is not set. --> 

<button [attr.shiny]="true"></button>
<!-- <button shiny></button> -->

We could also opt-in with:

<button [attr.bool.shiny]="isShiny"></button>
<!-- <button shiny="true"></button> -->
<!-- <button shiny="false"></button> -->

<button [attr.bool.shiny]="myObject"></button>
<!-- !!! throws a compile time error: myObject is not a Boolean. You can only use a Boolean value with [attr.bool.<foo>] !!! -->

As a bonus you could extend (in the future) the attribute binding with e.g. toJSON 😅

<button [attr.json.data-hero]='myObject'></button>
<!-- <button data-hero='{"name": "Batman", "realName": "Bruce Wayne"}'></button> -->

BTW, is it not possible to check at compile time if it is a Boolean or an object (I have no idea which code you are generating and if Dart allows this)?

So this (just brainstorming):

<button [attr.shiny]="myObject"></button>
<!-- <button></button> -->
<!-- Maybe a warn?: [attr.shiny] can only handle Boolean values. myObject is an object, therefore the attribute 'shiny' is not set. --> 

... generates something like this:

try {
  if (attrShinyValue) {
     // logic to add the attribute to the HTML element
  }
} catch (e) {
   // ups, not a Boolean
   // inform the user with a warn/error message
}

@zoechi
Copy link

zoechi commented Mar 13, 2018

Honestly I don't see any of the proposals carry it's weight.
I'd just leave it as it is.
There is no missing functionality as far as I am aware and
changing that behavior now for alternatives that don't seem to have a clear advantage
isn't worth it.

@akaufmann
Copy link

akaufmann commented Mar 13, 2018

Yeah, just a suggestion because they talked about it. Just gave my two cents.

If I would start from scratch (developing my Angular) I would in line it with the rest and because not everything has to be groundbreaking, but sometimes only to make the product a little more coherent.

@matanlurey
Copy link
Contributor Author

@akaufmann:

I think the best way would be to make it a breaking change

We've decided we are not adding any more (intentional) breaking changes to the framework before 6.0.0, so if we do add this it needs to be incremental (i.e. non-breaking).

@zoechi:

Honestly I don't see any of the proposals carry it's weight.

There is no missing functionality as far as I am aware.

There is, though. Users have not found a way to add/remove a value-less attribute. We could argue that isn't useful, but at least someone thinks it is, and they can't write the code today.

@zoechi
Copy link

zoechi commented Mar 14, 2018

@matanlurey

not found a way to add/remove a value-less attribute

Not sure what you mean by that. Currently Angular creates disabled="true" and the ="true" part can't be got rid of. That doesn't seem to be a real issue though. Still wondering why you mention "value-less attribute"

Have you checked if [attr.foo]="null" results in the attribute not being added (being removed).
That always worked for me, but I haven't used it since quite a while.

If this doesn't work, then there is of course work necessary.
If it does though, it's not clear what exactly the feature request is about.

@matanlurey
Copy link
Contributor Author

Yes, they want to omit the ="true" part, which is not idiomatic.

@zoechi
Copy link

zoechi commented Mar 14, 2018

I see.
Never saw real complaints about that in Angular TS world, just tons of questions how to get a bound attribute removed. One of my most successful answer on SO :D https://stackoverflow.com/questions/36745734/how-to-add-conditional-attribute-in-angular-2

In this case I'd prefer

 [attr.bool.shiny]

or if possible even better just

 [bool.shiny]

It could also be boolean.shiny because the bool is not related to the Dart type but to HTML "boolean attributes" (no strong opinion here)

I don't know about the amount of work which approach causes, but I think boolean attributes are special enough (but probably rather rare) so that their own prefix would be justified.
I don't think there is much difference in discoverability.
Even the null to get normal attributes removed is pretty hard to discover as the many upvotes to my SO question demonstrate.

@matanlurey
Copy link
Contributor Author

I'll let @TedSander and @nshahan weigh in because its their team's request.

@TedSander
Copy link
Contributor

We see this code pattern a good amount of time:
[attr.raised] = "raised ? '' : null"

It is weird to use an API where you ask the user to use null instead of false. False is more natural value. Will also be better for non-nullablity. Seems like we could do better. It was so bad teams were trying to add Inputs/unperformant code just to get around writing this syntactic sugar in their template.

I would think it would be an easy nice to use syntactic sugar that could be added pretty easily. If it is hard to do in the compiler we don't NEED it, but if it is easier I think it has been shown by other frameworks and the code we see inside that it is useful.

@zoechi
Copy link

zoechi commented Mar 14, 2018

@TedSander what about the DOM result raised="" instead of just raised? Does it matter?

I don't see much value in raised="false" (could always be achieved if necessary by [attr.raised]="'false'" anyway), therefore an expression result of falseremoving the attribute (the same asnull`) seems reasonable to me.

I have no strong opinion either way. I'm just curious about this topic because I was involved a bit in the discussion back then with Angular TS (and I think I also argued for false removing the attribute)

@nshahan
Copy link
Contributor

nshahan commented Mar 14, 2018

I see this as convenient when you want have styles that target the presence of an attribute on an element in the dom.

my-component {
  [my-attribute] {
    // some special styles
  }

It gets confusing when you have to deal with the attribute being present, not present, present but "true", and present but "false". It would be easier if "true" and "false" were always represented as present or not present in the dom.

I think it would be nice to have syntactic sugar that would allow you to have an input binding between a dart bool and the presence of an attribute on the element.

That way, if you wrote <my-component my-attribute>...</my-component> the input would be true. If the attribute isn't present then the bool in dart would be false.

At the same time if you set the value of the input to true, the attribute will be present, if you set it to false then the attribute will be removed entirely.

@TedSander
Copy link
Contributor

@zoechi raised="" vs raised doesn't matter. Most HTML APIs have the behavior that any value on the attribute means it is true.

This still autofocuses for example:
<input type="text" name="fname" autofocus="false">

Mostly we get to work around this because we deal with the JS property instead of the html attribute in angular, but we still need to deal with it for some HTML specific APIs like CSS as Nick mentioned, and other HTML like things.

@matanlurey matanlurey self-assigned this Jun 26, 2018
@matanlurey
Copy link
Contributor Author

We're moving forward here, non-breaking:

[attr.disabled.if]="isDisabled" == [attr.disabled]="isDisabled ? '' : null"`

(This is some-what similar to style.width.px, in-that it modifies the binding itself instead of toString)

It would have been nice to just do [attr.disabled]="isDisabled", but:

  • That's yet-another breaking change
  • It won't work properly in contexts like [attr.disabled]="checkA(thing)".

We can always revisit making this implicit in a future major release or behind a flag.

@matanlurey matanlurey changed the title Add support for [attr.if.name]="boolean" syntax Add support for [attr.name.if]="boolean" syntax Jun 27, 2018
@matanlurey
Copy link
Contributor Author

This was quite easy, so its making it in for 5.0.0 final!

@matanlurey
Copy link
Contributor Author

Closed in #1439.

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

5 participants