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

Edit typing of selectItem to allow to set null #1149

Closed
wants to merge 4 commits into from

Conversation

fabb
Copy link
Contributor

@fabb fabb commented Aug 14, 2020

What:

This PR enables to call selectItem(null) without having to typecast this at the call site in Typescript strict mode.

Why:

When items is a controlled prop, but selectedItem is not, it is necessary to manually reset the selectedItem after an action like SelectedItemClick has been executed.

How:

Simple ;-)

Checklist:

  • Documentation N/A
  • Tests - not sure if tests are necessary for a types-only change
  • TypeScript Types
  • Flow Types
  • Ready to be merged

In the example at https://www.downshift-js.com/use-combobox, selectItem(null) is used, so I added null and not undefined.

fabb added 2 commits August 14, 2020 18:44
When `items` is a controlled prop, but `selectedItem` is not, it is necessary to manually reset the selectedItem after an action like `SelectedItemClick` has been executed. This PR allows to call `selectItem(undefined)` without having to typecast this at the call site in Typescript strict mode.
@fabb fabb changed the title Edit typing of selectItem to allow to set undefined Edit typing of selectItem to allow to set null Aug 14, 2020
@@ -209,7 +209,7 @@ declare module downshift {
openMenu: (cb?: Callback) => void;
closeMenu: (cb?: Callback) => void;
toggleMenu: (otherStateToSet?: {}, cb?: Callback) => void;
selectItem: (item: Item, otherStateToSet?: {}, cb?: Callback) => void;
selectItem: (item: ?Item, otherStateToSet?: {}, cb?: Callback) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i never used flow. is this correct syntax? i have copied it from here: https://flow.org/en/docs/types/maybe/

but maybe (hah), rather null should be explicitly stated instead of a maybe type to disallow undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i changed it to | null and also edited the state type as well to include null for selectedItem, that was missing from the flow types, it was there for the typescript types already

…n typescript types

and make `selectItem` parameter explicitly `| null` to exclude `undefined`
@silviuaavram
Copy link
Collaborator

Don't you have reset for this?

@fabb
Copy link
Contributor Author

fabb commented Aug 14, 2020

Hm, maybe. I just fear it would reset too much, would need to try.
Still, the type changes make sense anyways since setting null is possible and has a desirable effect in some use cases.

@silviuaavram
Copy link
Collaborator

I am all for flexibility, but here I think it's a little bit too much. So I am going to ask for a use you can only use selectItem(null) and not reset().

Thank you for the feedback!

@fabb
Copy link
Contributor Author

fabb commented Aug 15, 2020

I have not yet tried it! The documentation of reset just says it resets to sensible defaults, this is a bit vague.

Also selectItem(null) would fit nicely with the other state setters, e.g. setHighlightedIndex(-1) is possible too. And it‘s even part of an example: https://www.downshift-js.com/use-combobox

@levenleven
Copy link

Examples on https://www.downshift-js.com/use-combobox use selectItem(null) to clear selection, but typings do not allow this

@silviuaavram
Copy link
Collaborator

Is this something that might be a breaking change? Adding another possiblity (null) shouldn't break anything in someone's typings, am I correct?

@levenleven
Copy link

levenleven commented Feb 1, 2021

Change in typescript typings is not breaking (function will just allow wider input)
Change in flow typings selectedItem: Item | null; in DownshiftState is kinda breaking, but I think still right thing to do (item indeed can be null)

@silviuaavram
Copy link
Collaborator

Then will keep this up for a major version bump.

@silviuaavram
Copy link
Collaborator

Will add these changes in #1415.

@silviuaavram silviuaavram mentioned this pull request Oct 21, 2022
silviuaavram added a commit that referenced this pull request Oct 22, 2022
BREAKING CHANGE: updates to useCombobox and useSelect to adhere to the 1.2 version of the ARIA Combobox pattern.

Migration guide is available in [this file](https://github.com/downshift-js/downshift/tree/master/src/hooks/MIGRATION_V7.md).

Closes #1365.
Closes #1239.
Contains changes from #1149.
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