Skip to content

Conversation

Guymestef
Copy link

What issue does this pull request resolve?
#597

What changes did you make?

  • Add shouldSelectHint on Typeahead component and pass it down to TypeaheadInputSingle and TypeaheadInputMulti

Is there anything that requires more attention while reviewing?

  • Cannot install node-sass because of a python gyp error. So since node-sass is deprecated, I replaced it with the new package sass which works on node without the need of python
  • Modify css filename computation to work on windows OS too

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #598 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #598   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          44       44           
  Lines         661      661           
  Branches      134      134           
=======================================
  Hits          660      660           
  Misses          1        1           
Impacted Files Coverage Δ
src/components/Hint.js 100.00% <ø> (ø)
src/components/TypeaheadInputMulti.js 96.42% <ø> (ø)
src/components/TypeaheadInputSingle.js 100.00% <ø> (ø)
src/components/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 e4028ce...d6de235. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.849% when pulling d6de235 on Guymestef:master into e4028ce on ericgio:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.849% when pulling d6de235 on Guymestef:master into e4028ce on ericgio:master.

@ericgio
Copy link
Owner

ericgio commented Nov 5, 2020

Thanks for the PR @Guymestef! I'm not convinced it's necessary to add shouldSelectHint as a top-level prop since it's already available on inputProps (see my comment on #597). I do like your suggestion to use the sass package though. Would you mind creating a separate PR for that?

@Guymestef
Copy link
Author

Thanks for the PR @Guymestef! I'm not convinced it's necessary to add shouldSelectHint as a top-level prop since it's already available on inputProps (see my comment on #597). I do like your suggestion to use the sass package though. Would you mind creating a separate PR for that?

Done!

@ericgio
Copy link
Owner

ericgio commented Nov 5, 2020

Closing this PR in favor of #599.

@ericgio ericgio closed this Nov 5, 2020
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