Skip to content

Add overloads of {add,remove}EventListener with options objects #343

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

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

busti
Copy link
Contributor

@busti busti commented Feb 5, 2019

This Pull Request adds an implementation for passing an options object to addEventListener.
The three options in that object allow for more control about how an event-listener behaves and can be quite useful, especially when working with touchscreen devices.

The documentation was provided by MDN, details about the standard can be read here:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters

The standard is supported by all major browsers, with the only exception being IE11.
https://caniuse.com/#feat=passive-event-listener

@busti
Copy link
Contributor Author

busti commented Feb 8, 2019

I do not know what causes the failure here.
Running the command on my machine is successful:

[IJ]> scalafmtTest
[info] Running org.scalafmt.cli.Cli --test
All files are formatted with scalafmt :)
[success] Total time: 0 s, completed Feb 8, 2019, 11:26:05 PM

@busti busti force-pushed the addEventListener-options branch from c861984 to cac92ee Compare February 18, 2019 21:46
@ramnivas
Copy link
Contributor

After making PR #348, I realized that there is this PR already pending. However, PR #348 also adds removeEventListener version that takes options. So either this PR should be amended with that addition or #348 should be considered as a superset of this.

@busti busti force-pushed the addEventListener-options branch 2 times, most recently from 584607f to 083f3cc Compare February 21, 2019 02:09
@busti
Copy link
Contributor Author

busti commented Feb 21, 2019

@ramnivas Thanks a lot for the heads up. That would have been confusing if people tried to remove that event listener.
I have applied the fixes to this PR as well, to make the decision a bit harder for @sjrd.
Choose whichever you like more. They are mostly identical except for some slight differences in the documentation.

@busti busti force-pushed the addEventListener-options branch 2 times, most recently from 0c43ad2 to 71eef64 Compare February 21, 2019 02:23
@ramnivas
Copy link
Contributor

I have closed #348, since this PR came first and #348 is almost identical, so this should take the precedence.

I think you should change the commit message to include "removeEventListener" as well.

@ramnivas
Copy link
Contributor

@busti BTW, you can run sbt scalafmt to fix the formatting errors seen in CI.

@busti
Copy link
Contributor Author

busti commented Feb 22, 2019

@ramnivas The problem is that scalafmt does not seem to work correctly for me.
I have tried running the commands that fail in travis on 3 different machines and in 3 different operating systems now.
I even created a clean debian installation in a VM and still...

Every time I run scalafmtTest it just says All files are formatted with scalafmt :)

@ramnivas
Copy link
Contributor

@busti hmm... May be you want to jiggle your code, perhaps by re-formatting comments (they use too many lines, IMO, and could be more readable by merging lines, anyway; see my closed PR for example). Then maybe scalafmt will be awaken!

@busti
Copy link
Contributor Author

busti commented Feb 23, 2019

@ramnivas I think the comments should be fine. They were too long before, that's why I have created more line-breaks. See: #343 (comment)

I have now tried running scalafmtTest on:

  • My Personal Computer (Windows IntelliJ)
  • My Personal Computer (WSL Debian)
  • My Work Computer (Windows IntelliJ)
  • My Work Computer (WSL Debian)
  • My Laptop (Arch)
  • The University Desktop (Debian)

I do not know what else to do here. I am using scalafmt in several other projects and it is working as expected.
From what it sounds like, scalafmt does work correctly on your systems @ramnivas & @sjrd .

If it doesn't bother you too much I would like to ask again, for you to check the formatting errors on one of your systems and tell me what's wrong.
Thanks a lot in advance.

@ramnivas
Copy link
Contributor

For me it has been working just fine.

@busti Probably you already did this, but did you try sbt scalafmt (i.e. not sbt scalafmtTest).

My suggestion above is to give some ill/differently-formatted code and let sbt scalafmt do what it could do. All else fails, copy docs from my PR (which passed all the checks). If even that fails, may be I can resurrect my PR in favor of this to unblock.

@sjrd
Copy link
Member

sjrd commented Feb 26, 2019

scalafmt gives the following diff on my computer:

diff --git a/src/main/scala/org/scalajs/dom/raw/lib.scala b/src/main/scala/org/scalajs/dom/raw/lib.scala
index 6fe8218..cf591ac 100644
--- a/src/main/scala/org/scalajs/dom/raw/lib.scala
+++ b/src/main/scala/org/scalajs/dom/raw/lib.scala
@@ -2737,8 +2737,8 @@ object EventListenerOptions {
    */
   @inline
   def apply(capture: js.UndefOr[Boolean] = js.undefined,
-    once: js.UndefOr[Boolean] = js.undefined,
-    passive: js.UndefOr[Boolean] = js.undefined): EventListenerOptions = {
+      once: js.UndefOr[Boolean] = js.undefined,
+      passive: js.UndefOr[Boolean] = js.undefined): EventListenerOptions = {
     val result = js.Dynamic.literal()
 
     capture.foreach(result.capture = _)
@@ -2797,8 +2797,8 @@ class EventTarget extends js.Object {
    * MDN
    */
   def removeEventListener[T <: Event](`type`: String,
-    listener: js.Function1[T, _],
-    options: EventListenerOptions): Unit = js.native
+      listener: js.Function1[T, _],
+      options: EventListenerOptions): Unit = js.native
 
   /**
    * The EventTarget.addEventListener() method

@busti busti force-pushed the addEventListener-options branch from 71eef64 to ca5372b Compare February 26, 2019 21:21
@busti
Copy link
Contributor Author

busti commented Feb 26, 2019

Thank you @sjrd again for using scalafmt for me.
I am sorry for the hold-up and any inconveniences I might have caused.

I have failed to mention that I have previously used scala, sbt and scalafmt on all of the above systems.
It is about time that I install a fresh OS on my Laptop anyways. So maybe then scalafmt will work correctly.

…nd removeEventListener

Create removeEventListener function which accepts an options object

Create companion object to EventListenerOptions with an initializer

Formatting fixes for scalafmt

Annotate companion instantiator with @inline

Indent lines to satisfy scalafmt

Remove EventListenerOptions object and apply method
@busti busti force-pushed the addEventListener-options branch from ca5372b to 1f716f5 Compare March 4, 2019 16:13
@busti
Copy link
Contributor Author

busti commented Mar 4, 2019

Sorry that this is so late. I had a rather eventful weekend and forgot about it.

@sjrd sjrd changed the title Add EventListenerOptions trait and implement it in addEventListener Add overloads of {add,remove}EventListener with options objects Mar 5, 2019
@sjrd sjrd merged commit a2bdc55 into scala-js:master Mar 5, 2019
@busti busti deleted the addEventListener-options branch March 5, 2019 16:39
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.

3 participants