iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
83 stars 23 forks source link

DatePicker: onChange should not have date argument as optional #426

Closed veekeys closed 2 years ago

veekeys commented 2 years ago

image

This does not really make sense as on change will always return date. Also, it makes usage of date picker not nice as you need to save variable with Date | undefined type. Just this change might be done only on v2 as it looks like a breaking one..

mayank99 commented 2 years ago

How is it a breaking change? You can make it not optional and it should not change usage.

You would still be able to do this:

onChange={() => {}}

Here's an example from Select (the value arg is not optional): image https://github.com/iTwin/iTwinUI-react/blob/b863b1d8917be9b64954bce03d75f60eb5f80d7a/src/core/Select/Select.tsx#L69

and here is one from Button: image

veekeys commented 2 years ago

Simply.

image

image

mayank99 commented 2 years ago

That would still not be a breaking change. If the users are already setting type of their state as Date | undefined, then narrowing down the type would still be valid.

veekeys commented 2 years ago

So breaking the build after library update now is not considered as breaking change?

veekeys commented 2 years ago

Ok, might be not a breaking change after all... We can make this fix.