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

fix: UseComboboxGetComboboxPropsOptions typing #1193

Conversation

clementgarbay
Copy link
Contributor

What:

I fixed the wrong typing of the UseComboboxGetComboboxPropsOptions interface. The generic type of the HTMLProps extend was HTMLLabelElement instead of HTMLDivElement.

Why:

From the related documentation (https://www.downshift-js.com/use-combobox), the DOM element where the getComboboxProps should be spread is div.

Checklist:

  • Documentation
  • Tests
  • TypeScript Types
  • Flow Types
  • Ready to be merged

@silviuaavram
Copy link
Collaborator

Rats! You're right, thanks you very much!

I see the build system is not feeling well. 58:21 error userEvent.typedoes not needawait operator testing-library/no-await-sync-events

I probably need to update the dependencies and fix this error before merging anything.

Thank you again for the contribution!

@silviuaavram
Copy link
Collaborator

@clementgarbay can you rebase this PR? It should pass now. Thank you very much!

One question: this fix can't be considered a breaking change right? I don't think it's very probable for someone to use label related field without them working.

@clementgarbay clementgarbay force-pushed the fix/combobox-props-options-type branch from 1f326e2 to 2f37832 Compare December 13, 2020 18:01
@codecov-io
Copy link

Codecov Report

Merging #1193 (2f37832) into master (cdfb1d2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1193   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1705      1705           
  Branches       493       493           
=========================================
  Hits          1705      1705           

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 cdfb1d2...2f37832. Read the comment docs.

@clementgarbay
Copy link
Contributor Author

@silviuaavram I just rebased and the build seems now fine! I don't think this fix should be considered as a breaking change

@silviuaavram silviuaavram merged commit 5e9f2c9 into downshift-js:master Jan 10, 2021
@silviuaavram
Copy link
Collaborator

@all-contributors please add @clementgarbay for code

@allcontributors
Copy link
Contributor

@silviuaavram

I've put up a pull request to add @clementgarbay! 🎉

@github-actions
Copy link

🎉 This PR is included in version 6.0.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants