Fluentui: Inconsistent onChange/onChanged prop types

Created on 15 Jan 2017  路  12Comments  路  Source: microsoft/fluentui

Bug Report

  • __Package version(s)__: 0.85.0

Describe the issue:

Some components have an onChange prop e.g. Searchbox, Slider, Checkbox while others use onChanged e.g. TextField, Toggle, DropDown

Actual behavior:

Prop types are not consistent

Expected behavior:

Prop types should be consistent

Normal

Most helpful comment

@scriby I am having the same issue here. I wanna get the attribute name and the value from the TextField react component, but the onChange is not firing

All 12 comments

From recent PRs, like #817, it looks like onChange is the preferred name. Let's deprecate any remaining instances of onChanged and replace them with onChange.

Yes. onChange with ev being the first param and custom things after, all as optionals.

This confused me just now with the TextField because I was trying to use the onChange event, which doesn't get fired. It's also not a compilation error because it exists intrinsically.

@scriby I am having the same issue here. I wanna get the attribute name and the value from the TextField react component, but the onChange is not firing

I have got the same issue. OnChange is not firing. Instead onChanged is firing but only with the entered text as parameter.
Returning the component proxy would be better.

Any update on this?

I'm looking into the best way to do this without adding breaking changes. I'm thinking: on the components that accept onChanged adding onChange that falls back to onChanged, then deprecating onChanged for next major release.

@jordandrako thanks!
My first thoughts were:

  • Having onChange which acted as usual (receiving proxy, not a value)
  • onChanged... well folks can stop using it after a while when onChange will arrive

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Fabric React!

We should fix this!

This issue has been automatically marked as stale because it has not activity for 30 days. It will be closed if no further activity occurs within 14 days of this comment. Thank you for your contributions to Fabric React!
Why am I receiving this notification?

I did a quick scan on which components use onChange:

| Name |Signature |
|-------|----------|
| Checkbox | onChange: (ev, checked) => void; |
| ChoiceGroup | onChange: (ev, option) => void; |
| pickers | onChange: (items) => void; |
| SearchBox | onChange: (newValue) => void; |
| Slider | onChange: (value) => void |

It seems that onChange parameters are not consistently ev, value. This is makes things rather confusing. We should stick with one or the other...

Regarding non-deprecated onChanged instances to be cleaned up:

| Name |Signature |
|-------|----------|
| ColorSlider | onChanged: (newValue) => void |
| ComboBox | onChanged: (option, index, value, submitPendingValueEvent) => void |
| Dropdown | onChanged: (option, index) => void |
| Rating | onChanged: (rating) => void |
| TextField | onChanged: (newValue) => void |
| Toggle | onChanged: (checked) => void |
| SelectableDroppableText | onChanged: (option, index) => void` |

So the work here:

  1. Add onChange(ev, value) to components missing it, deprecate onChanged.
  2. For ones that implement onChange without ev, this is way more sensitive. That would be a major breaking change. We can consider a codemod and major release for this.
Was this page helpful?
0 / 5 - 0 ratings