Skip to content

Trigger 'input' event before 'change' events #4649

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.

Sign up for GitHub

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

Jump to bottom
Merged
kevin-brown merged 1 commit into from
Jan 28, 2020
Merged

Trigger 'input' event before 'change' events #4649

kevin-brown merged 1 commit into from
Jan 28, 2020

Conversation

@marcandre
Copy link
Contributor

@marcandre marcandre commented Oct 24, 2016

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

  • Triggers an input event before triggering a change event
  • Tests looking for change event being triggered now look for input then change.

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

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

@marcandre marcandre mentioned this pull request Oct 24, 2016
Closed
kevin-brown
kevin-brown previously requested changes Nov 14, 2016
View reviewed changes
Copy link
Member

@kevin-brown kevin-brown left a comment

Choose a reason for hiding this comment

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

It's not clear under what cases the input event is fired. Is there a use case for the input event that is not dependent on the change event, or is one basically a copy of the other?

tests/data/select-tests.js

assert.ok(
changeTriggered,
'The change event should be triggered'
Copy link
Member

@kevin-brown kevin-brown Nov 14, 2016

Choose a reason for hiding this comment

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

I'm not sure how I feel about enforcing the order of the events within past tests. I can understand adding tests to check that the input event is triggered, and adding tests to ensure that one event is caught before the other, but I don't think that requires you to modify old change-only tests to cover both those cases.

@marcandre
Copy link
Contributor Author

marcandre commented Nov 14, 2016 •
edited
Loading

Thanks for taking a look at this PR 😄

  1. Every change event should be preceded by at least one input event. Most inputs have a single input event triggered just before a change event, but some like type="text" usually have many input events (e.g. after each key / paste / ...) before a single change event (when loosing focus). Another input with multiple input events before each change is the type="color".

  2. For tests, we could have one test to insure that change is triggered, another to check that input is triggered, and a third one to insure that they are triggered in the right order, but the first two are made redundant by the third one so that why I felt this was the best solution. Note that the order of the event is mandated by the spec.

Let me know how I can be of further assistance

@marcandre
Copy link
Contributor Author

marcandre commented Nov 13, 2018

@kevin-brown Is there something else that is blocking this PR from being merged?

@kevin-brown kevin-brown dismissed their stale review November 19, 2018 02:25

It's been two years.

@marcandre
Copy link
Contributor Author

marcandre commented Dec 2, 2018

I've rebased my branch.

@stale
Copy link

stale bot commented Mar 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Mar 13, 2019
@marcandre
Copy link
Contributor Author

marcandre commented Mar 13, 2019

This PR is really simple, and is still valid.

@stale stale bot removed the status: stale label Mar 13, 2019
@marcandre marcandre mentioned this pull request Mar 15, 2019
Closed
@stale
Copy link

stale bot commented May 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label May 12, 2019
@marcandre
Copy link
Contributor Author

marcandre commented May 12, 2019

This PR is really simple, and is still valid.

@stale stale bot removed the status: stale label May 12, 2019
@Marco-Sulla
Copy link

Marco-Sulla commented May 12, 2019

Please merge this pull request, is simple and adhere to the standards

@stale
Copy link

stale bot commented Jul 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Jul 11, 2019
@marcandre
Copy link
Contributor Author

marcandre commented Jul 11, 2019

Stale bot: 0
Me: 2

@stale stale bot removed the status: stale label Jul 11, 2019
@stale
Copy link

stale bot commented Sep 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Sep 9, 2019
@stale stale bot removed the status: stale label Sep 10, 2019
@marcandre
Copy link
Contributor Author

marcandre commented Sep 10, 2019

Rebased.

@kevin-brown kevin-brown added this to the 4.0.12 milestone Oct 14, 2019
@kevin-brown kevin-brown changed the base branch from master to develop October 14, 2019 03:02
@kevin-brown kevin-brown modified the milestones: 4.0.12, 4.0.13 Nov 6, 2019
@edwinvdpol
Copy link

edwinvdpol commented Dec 11, 2019

Ow come on, merge already!! 👍

@kevin-brown
Copy link
Member

kevin-brown commented Jan 28, 2020

Rebased this pull request against the latest dev so the new checks can run. The goal of this is to get it merged in tonight for the next patch release.

@kevin-brown kevin-brown merged commit 42364b1 into select2:develop Jan 28, 2020
@marcandre
Copy link
Contributor Author

marcandre commented Jan 28, 2020

Cool, thanks!

This was referenced Apr 8, 2020
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevin-brown kevin-brown kevin-brown left review comments

Assignees

No one assigned

Labels

4.x compatibility

Projects

None yet

Milestone

4.0.13

Development

Successfully merging this pull request may close these issues.

4 participants

@marcandre @Marco-Sulla @edwinvdpol @kevin-brown