search

jakezatecky / react-dual-listbox

A feature-rich dual listbox for React.
https://jakezatecky.github.io/react-dual-listbox/
MIT License
110 stars 59 forks source link

Updated typescript type definitions on DefinitelyTyped #294

Open N0NamedGuy opened 1 week ago

N0NamedGuy commented 1 week ago

Congratulations on this component. It is pretty nice component, that I have been using for some 3 years already (since v2.2.0), in a production context.

Now, in the project I am working on, we are migrating towards typescript. With the breaking changes introduce since, namely the new props, and the change of moveToLeft to moveToAvailable and such, the types in DefinitelyTyped have not been updated.

So, I took it up myself to update them, the best I could. Here is the PR https://github.com/DefinitelyTyped/DefinitelyTyped/pull/70899

If you do not wish to include the typescript definitions with your project, that is fine, and I will understand your decision (not everyone needs to like typescript in the javascript world...). However, if you do wish to ship the typescript definitions with your project, there they are.

I would appreciate any feedback you (or anyone else that is interested in this project) to take a look on the new type definitions.

PS: While updating the type definitions, I noticed that the onChange callback prop also receives a third parameter with all the component props. This is not documented. Was this intentional, or just missed?

PPS: I could not find any other easier way to reach you @jakezatecky ... So, I am sorry if this comes as spam to you...

Enjoy the rest of your day!

jakezatecky commented 1 week ago

Hey there.

Your PR looks fine, but since I do not use TypeScript or know all of its idiosyncrasies, I may not be the best person to provide feedback on these changes or update them in the future. Similarly, while I am not opposed to including TypeScript definitions in the project, I fear misalignment when there are API changes.

I think you mean filterCallback has an undocumented parameter. The onChange looks properly documented. Upon investigation, it looks like I introduced the third parameter to make use of the getOptionLabel parameter and allow others to access this (and potentially getOptionValue) instead of relying on option.label. We should probably update the documentation to reflect this.

Contacting me via issues is fine.