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

Input events #2697

Closed
wants to merge 2 commits into from
Closed

Input events #2697

wants to merge 2 commits into from

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Sep 21, 2016

While not widely known, the input event is triggered before all change events for inputs and selects (see the spec here)

This PR fixes this. Also adds a blurb on how to run tests...

I haven't changed the prototype version because it uses an old library that does not support the input event. Last commit was 4 years ago, looks like abandonware.

FWIW, Parsley relies on the input event, so the current code is causing bad interactions.

Thanks

@marcandre
Copy link
Contributor Author

PS: If you really want, it's easy to update the library, but then it has to be stated that only the one you specify has to be used...

Copy link
Collaborator

@koenpunt koenpunt left a comment

Choose a reason for hiding this comment

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

Thanks! See my code comments below (or above? where does this review thing puts them?)

@@ -510,3 +510,7 @@ class Chosen extends AbstractChosen
w = f_width - 10

@search_field.css({'width': w + 'px'})

form_field_has_changed: (extra) ->
Copy link
Collaborator

@koenpunt koenpunt Sep 22, 2016

Choose a reason for hiding this comment

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

Even if we're not going to support the input event in the prototype version I think we still should extract the change trigger to a method, just like is done here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also I'm not sure about the name of the method. More leaning to trigger_form_field_change, but maybe some of @harvesthq/chosen-developers can weigh in on this matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trigger_form_field_change was my first idea 😄, I thought it might be best to be a bit less specific in the name, but I'm fine with that name too.
You don't use _myPrivateMethod for private/protected methods, right? It's not really a public method.


#### Running tests

To install all development dependencies, in the project's root directory, run
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant, as it is already stated above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed. As an experienced dev, the only thing I'm interested in is how do I get to run the tests. For this reason, I thought a separate (and independant) section was better. I probably chose the wrong header size. Let me tweak it and split this into another PR.

@marcandre
Copy link
Contributor Author

OK, PR updated, with factorization done for prototype's version too.

@koenpunt
Copy link
Collaborator

Looks good, thanks!
@pfiller @stof @tjschuck second opinions?

@koenpunt
Copy link
Collaborator

@marcandre would it be easily possible to feature-test the prototype version to see if it has support for input?

@marcandre
Copy link
Contributor Author

The only way would be to know is to try to call simulate('input') and rescue an exception...

The lib isn't all that useful anyways... Want a PR to get rid of it?

@marcandre marcandre mentioned this pull request Sep 25, 2016
@marcandre
Copy link
Contributor Author

PR created, getting rid of simulate altogether.

@tjschuck
Copy link
Member

@koenpunt This looks good to me — if you're 👍 , I'm 👍

@tjschuck
Copy link
Member

@marcandre Do you mind squashing this down to one commit? Instructions if needed.

@marcandre
Copy link
Contributor Author

Closing in favor of #2704

@marcandre marcandre closed this Sep 27, 2016
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