Optionally relevant: Support for custom attributes on each option of a select. #1426

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
3 participants
Member

farmdawgnation commented Mar 26, 2013

This pull request implements the feature I described in this mailing list thread by allowing you to do things like disable particular options in a Lift-ed out select tag.

To use it, you'll do something like this...

val optionsForSelect =
  // regular option
  SelectableOption(someValue, "Label") ::
  // disabled option
  SelectableOption(someValue2, "Label2", "disabled" -> "disabled") ::
  Nil

...

selectObj(optionsForSelect, ...)

Additionally, there is an implicit conversion between (T, String) and SelectableOption[T], such that legacy code using the tuple format will continue to work, and those that don't need the additional attribute functionality can continue to use it.

Go over this with a fine-toothed comb. I'm almost certain I mixed up an nonce and a real value parameter somewhere, which we can all agree would be problematic (mostly because it would break things). I'll be going over it again soon, but my eyes need a break. Hah!

Also, this guy includes a few readability improvements elsewhere in the SHtml trait.

@Shadowfiend Shadowfiend and 1 other commented on an outdated diff Apr 2, 2013

web/webkit/src/main/scala/net/liftweb/http/SHtml.scala
@@ -1717,16 +1742,37 @@ trait SHtml {
SHtml.makeAjaxCall(LiftRules.jsArtifacts.serialize(formId), AjaxContext.js(Full(postSubmit.toJsCmd)))
- private def secureOptions[T](options: Seq[(T, String)], default: Box[T],
- onSubmit: T => Any): (Seq[(String, String)], Box[String], AFuncHolder) = {
- val secure = options.map {case (obj, txt) => (obj, randomString(20), txt)}
- val defaultNonce = default.flatMap(d => secure.find(_._1 == d).map(_._2))
- val nonces = secure.map {case (obj, nonce, txt) => (nonce, txt)}
- def process(nonce: String): Unit =
- secure.find(_._2 == nonce).map(x => onSubmit(x._1))
+ private def secureOptions[T](options: Seq[SelectableOption[T]], default: Box[T],
+ onSubmit: T => Any): (Seq[SelectableOption[String]], Box[String], AFuncHolder) = {
+ val secure = options.map {
+ case selectableOption =>
+ SelectableOptionWithNonce(selectableOption.value, randomString(20), selectableOption.label, selectableOption.attrs: _*)
@Shadowfiend

Shadowfiend Apr 2, 2013

Owner

Do we need this case? Since these are just objects, we can just use a parameter without the pattern match.

@farmdawgnation

farmdawgnation Apr 13, 2013

Member

Ah, right you are.

@Shadowfiend Shadowfiend and 1 other commented on an outdated diff Apr 2, 2013

web/webkit/src/main/scala/net/liftweb/http/SHtml.scala
- private def secureOptions[T](options: Seq[(T, String)], default: Box[T],
- onSubmit: T => Any): (Seq[(String, String)], Box[String], AFuncHolder) = {
- val secure = options.map {case (obj, txt) => (obj, randomString(20), txt)}
- val defaultNonce = default.flatMap(d => secure.find(_._1 == d).map(_._2))
- val nonces = secure.map {case (obj, nonce, txt) => (nonce, txt)}
- def process(nonce: String): Unit =
- secure.find(_._2 == nonce).map(x => onSubmit(x._1))
+ private def secureOptions[T](options: Seq[SelectableOption[T]], default: Box[T],
+ onSubmit: T => Any): (Seq[SelectableOption[String]], Box[String], AFuncHolder) = {
+ val secure = options.map {
+ case selectableOption =>
+ SelectableOptionWithNonce(selectableOption.value, randomString(20), selectableOption.label, selectableOption.attrs: _*)
+ }
+
+ val defaultNonce = default.flatMap { default =>
+ secure.find(_.value == default).map(_.nonce)
@Shadowfiend

Shadowfiend Apr 2, 2013

Owner

Is this a good candidate for collectFirst?

val defaultNonce =
  default flatMap { default =>
    secure collectFirst {
      case SelectableOptionWithNonce(value, nonce, _, _) if value == default =>
        nonce
    }
  }
@farmdawgnation

farmdawgnation Apr 13, 2013

Member

Does collectFirst have to be expanded out into multiple lines? secure.collectFirst(_.value == default).map(_.nonce) is easier to read, don't you think?

@Shadowfiend

Shadowfiend Apr 13, 2013

Owner

collectFirst lets you do basically a find + map together. I don't find it particularly more readable with both, but I personally prefer to use functions that are purpose-built for operations like this one.

I'm not too hung up on it though.

@farmdawgnation

farmdawgnation Apr 13, 2013

Member

Turns out that syntax for collectFirst doesn't work after all. I've glanced back and forth between the two and I really prefer the brevity of what I've already got. I'm probably going to leave it be.

@Shadowfiend Shadowfiend commented on an outdated diff Apr 2, 2013

web/webkit/src/main/scala/net/liftweb/http/SHtml.scala
- def process(nonce: String): Unit =
- secure.find(_._2 == nonce).map(x => onSubmit(x._1))
+ private def secureOptions[T](options: Seq[SelectableOption[T]], default: Box[T],
+ onSubmit: T => Any): (Seq[SelectableOption[String]], Box[String], AFuncHolder) = {
+ val secure = options.map {
+ case selectableOption =>
+ SelectableOptionWithNonce(selectableOption.value, randomString(20), selectableOption.label, selectableOption.attrs: _*)
+ }
+
+ val defaultNonce = default.flatMap { default =>
+ secure.find(_.value == default).map(_.nonce)
+ }
+
+ val nonces = secure.map {
+ case selectableOptionWithNonce =>
+ SelectableOption(selectableOptionWithNonce.nonce, selectableOptionWithNonce.label, selectableOptionWithNonce.attrs: _*)
@Shadowfiend

Shadowfiend Apr 2, 2013

Owner

Ditto on whether we need the case match.

@Shadowfiend Shadowfiend commented on an outdated diff Apr 2, 2013

web/webkit/src/main/scala/net/liftweb/http/SHtml.scala
+ onSubmit: T => Any): (Seq[SelectableOption[String]], Box[String], AFuncHolder) = {
+ val secure = options.map {
+ case selectableOption =>
+ SelectableOptionWithNonce(selectableOption.value, randomString(20), selectableOption.label, selectableOption.attrs: _*)
+ }
+
+ val defaultNonce = default.flatMap { default =>
+ secure.find(_.value == default).map(_.nonce)
+ }
+
+ val nonces = secure.map {
+ case selectableOptionWithNonce =>
+ SelectableOption(selectableOptionWithNonce.nonce, selectableOptionWithNonce.label, selectableOptionWithNonce.attrs: _*)
+ }
+
+ def process(nonce: String): Unit = secure.find(_.nonce == nonce).map(x => onSubmit(x.value))
@Shadowfiend

Shadowfiend Apr 2, 2013

Owner

This is probably a good candidate for collectFirst as well.

Member

farmdawgnation commented Apr 13, 2013

Soooooooo........ Do I need to close and reopen this against lift_30_dev now? Or are we thinking this may go in a 2.6 release?

Owner

fmpwizard commented Apr 13, 2013

I'd like to see this on 2.6

Owner

Shadowfiend commented Apr 13, 2013

Yeah, +1 for 2.6. But we need to get 2.5 out the door first… I'm wondering if we should have a 2.6 integration branch that we can put things in until 2.5 comes out…

Member

farmdawgnation commented Apr 13, 2013

That sounds like a good plan to me. lift_26_dev? :P

Owner

Shadowfiend commented Apr 13, 2013

If you don't intend on swapping find/map for collectFirst, I'd say go ahead and create the lift_26_dev branch, rebase these changes onto it, push it up, and then go ahead and close this ticket out.

Member

farmdawgnation commented Apr 14, 2013

This has been rebased on top of the new lift_26_dev branch. That branch has been version bumped to 2.6-SNAPSHOT as well. Posting info about it on the list now.

farmdawgnation deleted the optionally-relevant branch Apr 14, 2013

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