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

CurrentTarget / Target ops #139

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Conversation

mariusmuja
Copy link
Collaborator

This PR is a result of the discussion we had in #119 (comment). It still uses the non-typesafe cast for currentTarget, but allows for a nicer syntax and uses the TagWith... typeclasses for restricting the type the target can be cast to.

The syntax is the following:

  • currentTarget uses html.Input by default as the underlying event target:
input(
  onInput.value --> string,
  onClick.checked --> bool,
  onKeyDown.valueAsNumber --> number
)

The PR removes onInputString, onInputNumber and onInputChecked and updates the deprecation message on inputString, inputNumber and inputChecked to use onInput.value, onInput.valueAsNumber and onChange.checked instead.

  • it's possible to force a different event target for currentTarget, but only if the TagWith... typeclasses are implemented for it (html.TextArea, html.Select ).
textArea(
 onChange.value[html.TextArea] --> string,
)
  • for target, the type from TypedTargetEvent is used by default
input(
  onSearch.target.value --> stringStream,
  onSearch.target.valueAsNumber --> doubleStream,
  onSearch.target.checked --> boolStream,
)
  • in case the type of TypedTargetEvent is not specific enough it's possible to force a more specific type:
input(
  //onClick.target.value --> stringStream,   // does not compile because onClick is TypedTargetEvent[dom.Element], not specific enough
  onClick.target[html.Input].value --> stringStream,
  onClick.target[html.Input].valueAsNumber --> doubleStream,
  onChange.target[html.Input].checked --> boolStream,
)

@cornerman
Copy link
Member

Thanks for working on this. I did not have time to make a full review of the changes just yet.

But I do not get why we need these generic methods for currentTarget onChange.value[html.TextArea]. Just using onChange.value will behave the same way, as casting is unchecked in scalajs anyhow. What is the benefit of having it?

Also, why do we decide that currentTarget can assume html.Input, but target cannot? Here, we do not have typesafety anyhow.

@mariusmuja
Copy link
Collaborator Author

But I do not get why we need these generic methods for currentTarget onChange.value[html.TextArea]. Just using onChange.value will behave the same way, as casting is unchecked in scalajs anyhow. What is the benefit of having it?

Also, why do we decide that currentTarget can assume html.Input, but target cannot? Here, we do not have typesafety anyhow.

You are correct, there's no benefit from having the generic methods for casting to different event targets, they will behave the same way. I've removed them and simplified the PR.

@@ -426,4 +427,69 @@ class DomEventSpec extends JSDomSpec {
docClicked shouldBe true
}

"TagWith" should "correctly work on events" in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename TagWith to TargetOps in test string here

def checked: EmitterBuilder[E, Boolean] = builder.map(e => e.currentTarget.asInstanceOf[html.Input].checked)
}

implicit class TargetMap[E <: Event, O <: Event](builder: EmitterBuilder[E, O]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue against mapping to EventTarget here, as _.map(_.target) seems good enough to me and we do not have the same helper functions for other event properties. Instead we can only define a target object with the above methods in an implicit class for the EmitterBuilder.

Like this:

  implicit class TargetAsInput[E <: Event, O <: Event](builder: EmitterBuilder[E, O]) {
      object target {
          def value = ???
          def valueAsNumber = ???
          def checked = ???
      }
   }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll update the PR.

Copy link
Member

@cornerman cornerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR improves the handling for using currentTarget and target. For now, this is nice to use. But I am still unhappy that we do not have any kind of typesafety here - which we should definitely tackle in an upcoming PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants