Bye bye simulate #2704

marcandre · 2016-09-25T15:36:14Z

This PR removes the dependency on simulate, which is old, unmaintained and incomplete.

Replacing costs about 5 lines of code...

It also adds the input event to the prototype version.

Builds on #2697

koenpunt · 2016-09-25T18:32:16Z

👍 Looks good, will review in the morning.

koenpunt

In general I'm really pro the removal of the extra event dependency, so that's great, thanks! Just see my notes below.

koenpunt · 2016-09-26T10:20:57Z

coffee/chosen.proto.coffee

+    Chosen.trigger(@form_field, 'input')
+    Chosen.trigger(@form_field, 'change')
+
+  @trigger: (element, eventType) ->


Having trigger as public method shouldn't be necessary. But this probably has been done so the method is available in the tests as well?

Right. Should I call it _trigger instead?

koenpunt · 2016-09-26T10:22:37Z

coffee/chosen.proto.coffee

+    Chosen.trigger(@form_field, 'change')
+
+  @trigger: (element, eventType) ->
+    if (document.createEventObject) # Old IE:


if statements should be written without parens (I know, there are some conditions that do use parens, but they just slipped through the cracks I guess)

koenpunt · 2016-09-26T10:23:38Z

spec/proto/basic.spec.coffee

    expect(container.hasClassName("chosen-container-active")).toBe true

    #select an item
-    container.select(".active-result").last().simulate("mouseup")
+    Chosen.trigger(container.select(".active-result").last(), 'mouseup');


Nitpicky, but inconsistent quote usage. On multiple locations, but marking them all feels too extreme.

And yes I know it's hypocritical, because we use single and double quotes randomly around the library, but I still strive for some consistency.

👍 should not have switched them

Avoid ariya/phantomjs#11289

stof · 2016-09-27T16:48:19Z

spec/proto/basic.spec.coffee

@@ -27,11 +27,11 @@ describe "Basic setup", ->
    expect($F(select)).toBe ""

    container = div.down(".chosen-container")
-    container.simulate("mousedown") # open the drop
+    Chosen.trigger(container, "mousedown") # open the drop


this is not a good idea, because Chosen.trigger is not a full-fledged implementation of triggering builtin events (it always relies on the Event class, while many DOM events are using subclasses with extra properties)

So what is a better idea?

Well, as Chosen does not need a full-fledged event triggering system (as it does not need to trigger mouse events), the simpler would probably be to keep using an external library for that in the testsuite

I arrived at the same conclusion 😄

stof · 2016-09-27T16:52:17Z

coffee/chosen.proto.coffee

+  @trigger: (element, eventType) ->
+    if (document.createEventObject) # Old IE:
+      element.fireEvent("on#{eventType}", document.createEventObject());
+    else # Modern way:


you should detect support for the modern way instead (detecting dispatchEvent), to use the modern way when multiple ways are available. fireEvent should be a fallback only.

Btw, you should also write the code using createEvent() and initEvent when dispatchEvent is available but not the Event constructor (i.e. the intermediate API)

marcandre · 2016-09-27T16:53:20Z

Tweaked as per comments, and reorganized the commits to make more sense.

marcandre · 2016-09-27T18:02:43Z

Ok, hopefully third time's the charm.

No more Chosen.trigger, dependency on simulate is kept only for testing, and prototype version supports 5 year old Safari and al., while being forward compatible by not using initEvent and al. when possible (those are deprecated).

stof · 2016-09-28T09:57:56Z

coffee/chosen.proto.coffee

+        evt.initEvent(eventType, true, true);
+      element.dispatchEvent(evt)
+    else
+      document.createEventObject # Old IE:


this line is useless

Indeed. Fixed

stof · 2016-09-28T09:58:31Z

public/options.html

@@ -229,7 +229,7 @@
          <td>change</td>
          <td>
            <p>Chosen triggers the standard DOM event whenever a selection is made (it also sends a <code class="language-javascript">selected</code> or <code class="language-javascript">deselected</code> parameter that tells you which option was changed).</p>
-            <p><strong>Note:</strong> in order to use change in the Prototype version, you have to include the <a href="https://github.com/kangax/protolicious/blob/5b56fdafcd7d7662c9d648534225039b2e78e371/event.simulate.js">Event.simulate</a> class. The selected and deselected parameters are not available for Prototype.</p>
+            <p>The selected and deselected parameters are not available for Prototype.</p>


<strong>Note:</strong> should be kept

Except for tests

koenpunt · 2016-10-01T12:00:48Z

@stof anything else?

koenpunt · 2016-10-01T12:12:15Z

Thanks again, @marcandre 💯

marcandre · 2016-10-01T13:36:02Z

👍 I'm pretty impressed by the committer team!

marcandre · 2016-10-01T13:37:33Z

Is it possible to make a release with this?

koenpunt · 2016-10-01T13:39:28Z

I'm collecting as much small bugfixes as possible for a new release, but I expect to release 1.7.0 somewhere next week.

marcandre · 2016-10-19T18:37:03Z

I would still very much appreciate a release...

koenpunt · 2016-10-20T15:29:10Z

@marcandre I understand, and it has been more than a week now, but there are still quite some PR's that I'd like to see merged for the 1.7.0 release.
Truth is though that I can't make all decisions on my own, and not all maintainers are as active as I have been for the past month (which is fine, because open source), which causes that not everything goes as quick as I want it to go.
But if you like, reviews on the last 8 pull requests are more than welcome, which can eventually speed up the review process for the other maintainers.

koenpunt · 2017-03-24T20:46:18Z

@marcandre took some time, but as there was no progress since a few months I decided to put out a release. So 1.7.0 is now available.

marcandre · 2017-03-25T02:08:14Z

Thanks @koenpunt

marcandre force-pushed the bye_bye_simulate branch from e200c19 to 93f8cd8 Compare September 25, 2016 15:36

koenpunt requested changes Sep 26, 2016

View reviewed changes

marcandre added 2 commits September 27, 2016 12:48

Bump jasmine.

fd72ed7

Avoid ariya/phantomjs#11289

Factorize 'change' event trigger

29792b4

stof requested changes Sep 27, 2016

View reviewed changes

marcandre force-pushed the bye_bye_simulate branch from a308b1c to 1f300d5 Compare September 27, 2016 16:53

marcandre mentioned this pull request Sep 27, 2016

Input events #2697

Closed

marcandre force-pushed the bye_bye_simulate branch 2 times, most recently from a6b7f46 to 51bf72b Compare September 27, 2016 17:59

marcandre force-pushed the bye_bye_simulate branch from 51bf72b to 45d6883 Compare September 27, 2016 18:04

koenpunt approved these changes Sep 28, 2016

View reviewed changes

stof reviewed Sep 28, 2016

View reviewed changes

marcandre added 2 commits September 28, 2016 09:48

Don't depend on Simulate (prototype)

e1064eb

Except for tests

Trigger 'input' events before 'change' events

80a5d63

marcandre force-pushed the bye_bye_simulate branch from 45d6883 to 80a5d63 Compare September 28, 2016 13:48

stof approved these changes Oct 1, 2016

View reviewed changes

koenpunt merged commit 439a711 into harvesthq:master Oct 1, 2016

marcandre deleted the bye_bye_simulate branch October 1, 2016 13:37

marcandre mentioned this pull request Oct 20, 2016

Parsely 2.4.4 does not "see" change events when using custom <select> libraries like chosen guillaumepotier/Parsley.js#1134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bye bye simulate #2704

Bye bye simulate #2704

marcandre commented Sep 25, 2016 •

edited

Loading

koenpunt commented Sep 25, 2016

koenpunt left a comment

koenpunt Sep 26, 2016

marcandre Sep 27, 2016

koenpunt Sep 26, 2016

marcandre Sep 27, 2016

koenpunt Sep 26, 2016 •

edited

Loading

marcandre Sep 27, 2016

stof Sep 27, 2016

marcandre Sep 27, 2016

stof Sep 27, 2016

marcandre Sep 27, 2016

stof Sep 27, 2016

marcandre Sep 27, 2016

marcandre commented Sep 27, 2016

marcandre commented Sep 27, 2016

stof Sep 28, 2016

marcandre Sep 28, 2016

stof Sep 28, 2016

marcandre Sep 28, 2016

koenpunt commented Oct 1, 2016

koenpunt commented Oct 1, 2016

marcandre commented Oct 1, 2016

marcandre commented Oct 1, 2016

koenpunt commented Oct 1, 2016

marcandre commented Oct 19, 2016

koenpunt commented Oct 20, 2016

koenpunt commented Mar 24, 2017

marcandre commented Mar 25, 2017

Bye bye simulate #2704

Bye bye simulate #2704

Conversation

marcandre commented Sep 25, 2016 • edited Loading

koenpunt commented Sep 25, 2016

koenpunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenpunt Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre commented Sep 27, 2016

marcandre commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenpunt commented Oct 1, 2016

koenpunt commented Oct 1, 2016

marcandre commented Oct 1, 2016

marcandre commented Oct 1, 2016

koenpunt commented Oct 1, 2016

marcandre commented Oct 19, 2016

koenpunt commented Oct 20, 2016

koenpunt commented Mar 24, 2017

marcandre commented Mar 25, 2017

marcandre commented Sep 25, 2016 •

edited

Loading

koenpunt Sep 26, 2016 •

edited

Loading