ThoughtWorksInc / Binding.scala

Reactive data-binding for Scala
https://javadoc.io/page/com.thoughtworks.binding/binding_2.13/latest/com/thoughtworks/binding/index.html
MIT License
1.59k stars 108 forks source link

Optional attribute support #90

Closed glmars closed 5 years ago

glmars commented 5 years ago

In some library/wrapper code we need possibility to add attribute value conditionally. Something like this will be really helpfully:

  "OptionAttribute" in {
    val width = Var(scala.Option.empty[Int])

    @dom val td = <td width={width.bind}/>
    td.watch()

    assert(td.value.outerHTML == "<td/>")
    width.value = Some(100)
    assert(td.value.outerHTML == "<td width=100/>")
  }

But implementation such feature in Binding.scala can broke current code with custom tags (if somebody uses suggestion from #4 and has property with type like Option[T])

@Atry, do you think such Binding.scala feature will be useful? /cc @ccamel

Atry commented 5 years ago

How do you think about using undefined or null for optional DOM attributes?

glmars commented 5 years ago

Good options. But I just remembered about js.UndefOr[T]! Maybe this is the best candidate?

Atry commented 5 years ago

I think <image id={undefined}/> does not compile because the type is String: https://www.scala-js.org/api/scalajs-dom/0.9.5/index.html#org.scalajs.dom.raw.Element@id:String

Atry commented 5 years ago

By the way, I created anothor library to handle null: https://users.scala-lang.org/t/nullsafe-kotlin-groovy-flavored-null-safe-operator-now-in-scala/3244

glmars commented 5 years ago

Yes, current macros in Binding.scala doesn't support <image id={undefined}/> because trying to set value directly to property. We need change macros to something like: optionAttributeValue.foreach(v => currentTarget.currentProp = v) (in case of Option[T])

glmars commented 5 years ago

If you don't like an idea with single fixed in Binding.scala type for optional attribute value. How do you think about introducing some AttributeSetter[V, T] typeclass? In this case it will be possible to implement setters for some type pairs outside Binding.scala! Unfortunately, in my opinion it can increase the size of generated JS code.

glmars commented 5 years ago

Oh! the main problem is different. Unfortunately, it isn't possible to "unset" an attribute by using setter (because the type of id is String as you noticed) :( We need some dom specific code for it, for example by using https://www.scala-js.org/api/scalajs-dom/0.9.5/index.html#org.scalajs.dom.raw.Element@removeAttribute(name:String):Unit

Atry commented 5 years ago

I think macros are not necessary. Some implicit classes similar to DataOps would help https://github.com/ThoughtWorksInc/Binding.scala/blob/d0e099a45e882e1fc83d812fd330d43016ebf7fc/dom/src/main/scala/com/thoughtworks/binding/dom.scala#L170

Leonid Turnaev notifications@github.com 于2018年10月10日周三 上午11:01写道:

Oh! the main problem is different. Unfortunately, it isn't possible to "unset" an attribute by using setter (because the type of id is String as you noticed) :( We need some dom specific code for it, for example by using https://www.scala-js.org/api/scalajs-dom/0.9.5/index.html#org.scalajs.dom.raw.Element@removeAttribute(name:String):Unit

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ThoughtWorksInc/Binding.scala/issues/90#issuecomment-428421810, or mute the thread https://github.com/notifications/unsubscribe-auth/AAktuuScoax7Z-lfOAssgKLKlkNgCwGTks5ujWL7gaJpZM4XTpH5 .

-- 杨博 (Yang Bo)

glmars commented 5 years ago

I think I understand you. I am going to create PR (unfortunately, an option to assign this task to myself is disabled on this issue).

mcku commented 5 years ago

Assuming this feature exists in the 11.3.0 release, I have tried the example in the post but got the following error:

type mismatch;
 found   : Option[Int]
 required: Int
  @dom val td: Var[Element] = <td width={width.bind}/> 

How do you get it to work? Thanks.

glmars commented 5 years ago

Implementation is slightly different from the example in this issue: only Option[String] is supported, you should use option: prefix in an attribute name... please, see test from PR: https://github.com/ThoughtWorksInc/Binding.scala/pull/103/files#diff-1ee69eb8fca397f2ef7f3efd0f631c03