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

EditableText blur fires both onChange and onConfirm despite nothing changing #483

Closed
jrafidi opened this issue Jan 13, 2017 · 5 comments
Closed

Comments

@jrafidi
Copy link
Contributor

jrafidi commented Jan 13, 2017

Bug report

  • Package version(s): 1.3.1
  • Browser and OS versions: Chrome 55, OSX 10.11.6

Steps to reproduce

  1. Create an EditableText with some content and onChange/onConfirm handlers
  2. Focus the EditableText
  3. Blur the EditableText

Actual behavior

Without changing any of the content, the change/confirm handlers are called.

Expected behavior

Since nothing has changed, and nothing has been confirmed, I don't think it makes sense to call either of these handlers. Native inputs have onBlur, which I think would make more sense.

@cmslewis
Copy link
Contributor

Yeah, this makes sense. This is relevant to #189 (the NumericInput component) as well.

@leebyp
Copy link
Contributor

leebyp commented Feb 6, 2017

@jrafidi I'm reverting this change following the conversation in #573.
Specifically on your original issue

  • onChange is called because the state of the <EditableText /> itself has changed (from editing to not editing)
  • onConfirm is called because we treat all blur events except from esc as confirm events (keeping things consistent)

that being said, if you feel strongly about it, we can open another issue to see if introducing an onBlur would be helpful in v2.0. thoughts?

@giladgray
Copy link
Contributor

@jrafidi would it be sufficient to only fire onChange on blur if the value has changed? onConfirm should definitely be fired on blur regardless of value because of its semantics (escape key or blur).

@jrafidi
Copy link
Contributor Author

jrafidi commented Feb 8, 2017

@leebyp Don't really feel strongly about it either way.

@giladgray Sure seems fine to me. Contradicts @leebyp's point above regarding onChange including the state of the component changing from editing to not editing, but I think what you're saying makes more sense. I'd expect onChange to be related to the text value, not the overall state of the component as well.

@leebyp
Copy link
Contributor

leebyp commented Feb 10, 2017

closing with #629 merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants