Skip to content

Conversation

anonymous-ME
Copy link

@anonymous-ME anonymous-ME commented Mar 28, 2021

What issue does this pull request resolve?
onInputChange not called when input cleared. Fix for this issue #594

What changes did you make?
added the callBack

Is there anything that requires more attention while reviewing?
nope

@codecov
Copy link

codecov bot commented Mar 28, 2021

Codecov Report

Merging #630 (c1e21a3) into master (c50cbca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c1e21a3 differs from pull request most recent head 5e18602. Consider uploading reports for the commit 5e18602 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #630   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          44       44           
  Lines         661      663    +2     
  Branches      134      135    +1     
=======================================
+ Hits          660      662    +2     
  Misses          1        1           
Impacted Files Coverage Δ
src/core/Typeahead.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c50cbca...5e18602. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0005%) to 99.849% when pulling 5e18602 on anonymous-ME:master into c50cbca on ericgio:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0005%) to 99.849% when pulling 5e18602 on anonymous-ME:master into c50cbca on ericgio:master.

Copy link
Owner

@ericgio ericgio left a comment

Choose a reason for hiding this comment

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

Hey @anonymous-ME, thanks for submitting this PR. The issue you're trying to solve with this change is actually a bit more complicated than it first appears. onInputChange also passes the change event as the second argument, but that event doesn't actually happen, since the value changes in response to a click on a different element.


export function clearTypeahead(state: TypeaheadState, props: Props) {
if (typeof props.onInputChange === "function") {
props.onInputChange("");
Copy link
Owner

Choose a reason for hiding this comment

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

As noted, onInputChange passes the change event as the second argument. It also doesn't seem like a good idea to trigger a callback during a state change. That should happen after the state change is complete.

@ericgio
Copy link
Owner

ericgio commented Mar 28, 2021

Oh, and if you'd like to continue pursuing this change, some tests would be great :)

@anonymous-ME
Copy link
Author

Hey @anonymous-ME, thanks for submitting this PR. The issue you're trying to solve with this change is actually a bit more complicated than it first appears. onInputChange also passes the change event as the second argument, but that event doesn't actually happen, since the value changes in response to a click on a different element.

Hi @ericgio, what solution would you suggest in this case?

@anonymous-ME
Copy link
Author

Should we add a callback for onClear instead?

@ericgio
Copy link
Owner

ericgio commented Mar 29, 2021

I think the solution is that the event needs to be manually triggered by the clear callback, which would then trigger onInputChange as one would expect.

@anonymous-ME
Copy link
Author

Hi @ericgio, I'm thinking of doing something like this to simulate onChange for TypeaheadManager.

  const event = new Event('change');
  element.dispatchEvent(event);

But not sure how to get TypeaheadManager element reference inside of clearTypeahead callback.

@ericgio
Copy link
Owner

ericgio commented Aug 15, 2021

Thanks for having a look at this! The callback was added in v5.2

@ericgio ericgio closed this Aug 15, 2021
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