feat: add on change handlers and renamed others for standardisation #2293
Conversation
silviuaavram
requested review from
dvdzkwsk,
dzearing,
ecraig12345,
JasonGore,
jdhuntington,
kenotron,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Perf comparison
Potential regressions comparing to master
Perf tests with no regressions
Generated by 🚫 dangerJS |
| @@ -70,7 +70,7 @@ const checkOpenTitles = (wrapper: ReactWrapper, expected: string[]): void => { | |||
| } | |||
|
|
|||
| describe('Tree', () => { | |||
| isConformant(Tree) | |||
| isConformant(Tree, { autocontrolledPropMappings: { activeItemIds: 'onActiveItemIdsChange' } }) | |||
There was a problem hiding this comment.
Why not just adding a list of auto controlled props?
| @@ -34,6 +34,8 @@ export interface Conformant { | |||
| /** This component uses wrapper slot to wrap the 'meaningful' element. */ | |||
| wrapperComponent?: React.ReactType | |||
| handlesAsProp?: boolean | |||
| /** In the case when a onChange prop name is custom in relation to the controlled prop. */ | |||
There was a problem hiding this comment.
Comment is confusing; the way I read the implementation it seems like this key is always required, not just when the prop name is customized.
There was a problem hiding this comment.
oups, leftover from previous implementation, will correct that.
silviuaavram
changed the title
| @@ -432,7 +454,7 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo | |||
| const { highlightedIndex, open, searchQuery, value } = this.state | |||
|
|
|||
| return ( | |||
| <ElementType className={classes.root} {...unhandledProps}> | |||
| <ElementType className={classes.root} onChange={this.handleChange} {...unhandledProps}> | |||
There was a problem hiding this comment.
Aren't we going to end up here with onChange handler on a div element?
There was a problem hiding this comment.
I did the same thing as in RadioGroupItem. Just to pass the isConformant. I don't know any better way.
// RadioGroupItem component doesn't present any `input` component in markup, however all of our
// components should handle events transparently.
| @@ -144,21 +152,26 @@ class Menu extends AutoControlledComponent<WithAsProp<MenuProps>, MenuState> { | |||
| static Item = MenuItem | |||
| static Divider = MenuDivider | |||
|
|
|||
| setActiveIndex = (e: React.SyntheticEvent, activeIndex: number) => { | |||
| _.invoke(this.props, 'onActiveIndexChange', e, this.props) | |||
There was a problem hiding this comment.
Let's send here the new activeIndex together with the props?
| @@ -200,7 +200,7 @@ class RadioGroup extends AutoControlledComponent<WithAsProp<RadioGroupProps>, an | |||
| props: RadioGroupItemProps | |||
| }) { | |||
| this.setState({ checkedValue, shouldFocus }) | |||
| _.invoke(this.props, 'checkedValueChanged', event, props) | |||
| _.invoke(this.props, 'onCheckedValueChange', event, props) | |||
There was a problem hiding this comment.
Shouldn't this be onChange?
There was a problem hiding this comment.
If the prop was value, then yes. Are we changing everything here?
There was a problem hiding this comment.
I see, I thought it is the native value prop, but I see now that this is the RadioGroup component.
|
|
||
| _.invoke(predefinedProps, 'onSiblingsExpand', e, treeItemProps) | ||
| }, | ||
| }) | ||
|
|
||
| setActiveItemIds = (e: React.SyntheticEvent, activeItemIds: string[]) => { | ||
| _.invoke(this.props, 'onActiveItemIds', e, this.props) |
There was a problem hiding this comment.
Send the updated activeItemIds please together with the props.
| * @param event - React's original SyntheticEvent. | ||
| * @param data - All props and the new selected value(s). | ||
| */ | ||
| onActiveSelectedIndexChange?: ComponentEventHandler<DropdownProps> |
There was a problem hiding this comment.
Can we suggest in the comment if the event is null for some handler?
There was a problem hiding this comment.
As far as I see, the event might be null, depending from which place the handler was called. And not only for this one, also for onChange as well. What do you suggest?
There was a problem hiding this comment.
But we already have an issue regarding this: #2318 We should distinguish between the component event handlers that have event and the ones which do not. At least this is my opinion, we can ask other people for input
| @@ -124,7 +124,7 @@ class Embed extends AutoControlledComponent<WithAsProp<EmbedProps>, EmbedState> | |||
|
|
|||
| if (iframeNil || (!iframeNil && !this.state.active)) { | |||
| this.setState({ active: !this.state.active }) | |||
| _.invoke(this.props, 'onActiveChanged', e, { ...this.props, active: !this.state.active }) | |||
| _.invoke(this.props, 'onActiveChange', e, { ...this.props, active: !this.state.active }) | |||
There was a problem hiding this comment.
Can you save the value that we want to add in the state in a variable and reuse it in both setState and here? This is not save, as setState is async, so we should either move this into the callback in setState, or add here a variable's value, not the state's value
BREAKING CHANGES
Some onChange handlers have been renamed to become standardised with HTML elements and with the name of the prop they are representing. The list of changes:
Some additional handlers added:
isConformat tests have been improved. If a component has a state value as controlled prop, then it should also have a default prop and a onChange handler as handled props.
Fixes #2119
Some components could not be covered by the test as they are not conformant (Dialog, Tooltip, Popup etc.)