-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add on change handlers and renamed others for standardisation #2293
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups, leftover from previous implementation, will correct that.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we going to end up here with onChange
handler on a div
element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be onChange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the prop was value
, then yes. Are we changing everything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Send the updated activeItemIds
please together with the props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, to we want to include Dialog
, Popup
& Tooltip
there?
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we suggest in the comment if the event is null for some handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)