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

normalize range input onChange events #5687

Closed
wants to merge 1 commit into from
Closed

normalize range input onChange events #5687

wants to merge 1 commit into from

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Dec 17, 2015

Addresses the core of #554. ping @jimfb @spicyj

This plugin is getting large...personally I'd break it out into smaller modules but I didn't want to presume that here, so its all together. The other thing I didn't do was make the obvious DRY changes. There is a common pattern of tracking the active input used by three code branches that could be refactored into something a bit more concise. I have that change locally if ya'll feel like its appropriate for this PR otherwise I can leave the repetition.

There is also a larger question of whether it makes sense to just run all (or more than just ranges) inputs through this branch, which would also address annoyances similar to #3377 and #3484. That's a sweeping change though and, again, didn't want to presume.

I need to actually test this on old IE (Edge fires input events) but I don't have the vm set up at the moment.

@jquense
Copy link
Contributor Author

jquense commented Dec 21, 2015

the more I think about it (and maybe that was @spicyj's original intent) just deduping every change for all/most inputs would address a lot of the other edge case issues with onChange

I will PR something more comprehensive...first I want to first get a bunch of tests written around each inputs change behavior to catch any regressions

@jimfb
Copy link
Contributor

jimfb commented Mar 3, 2016

@jquense Would love to get a fix merged. What is the status on this? Are you blocking on us, or are we blocking on you? Feel free to ping me on google hangouts if that would be helpful.

@jquense
Copy link
Contributor Author

jquense commented Mar 3, 2016

@jimfb I think the ball is in your court, but its a bit complicated. The curent problem with this diff, is that this will only work when when inputs can be focused, i.e attached to the document, which isn't intuitive.

#5746 offers a more complete way to dedupe, but also applies to all inputs. If we want to localize a fix to just these inputs then I need to pull changes from that PR. I haven't gotten enough feedback though to know where and how to move forward (this one? this approach?).

Glad to chat more about it, around whenever, if we need to set something up.

@jimfb
Copy link
Contributor

jimfb commented Apr 1, 2016

Ok, if I understand this correctly (please correct me if I'm mistaken), #5746 is a superset of this PR. We've decided that we want to take that one, so I think this PR can be closed.

@jimfb jimfb closed this Apr 1, 2016
@jquense
Copy link
Contributor Author

jquense commented Apr 1, 2016

👍 yup this is safe to close

@facebook-github-bot
Copy link

@jquense updated the pull request.

@jquense jquense deleted the dedupe-range-events branch April 1, 2016 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants