Semantic-ui-react: RFC: all event handlers should callback with (event, data)

[craig1123]

Hi, I have a simple enhancement to propose.

It would be nice to have access to the options of a Dropdown using the onMouseEnter handler. There is obviously already an onChange handler which allows me to execute some function when I select an option, but I want to hover over the items as well, triggering some function.

Thanks, I really enjoy working with stardust.

[levithomason]

/cc @jcarbo @layershifter, speaking of _always_ passing props with all event handlers ^ 😸

@craig1123 Your timing was impeccable here, I was just suggesting to the team here that we should always pass (e, props) to every handler. Granted, we don't have an onMouseEnter handler in this component. We'd need to add any event handlers that should receive props.

I think this is useful and we should consider making it a library feature, and not just add it to this one component for this one handler. I'll leave it at that for now and suggest we start looking at how to achieve this consistently. Ideas welcomed.

[layershifter]

LGTM, I think we need there list of components with such events and update them all.

[levithomason]

_This is a living spec open to changes, feel free to comment for changes._ 💬

---

New Callback Parameters

Use event|null

React's SyntheticEvent should always be passed back, if available. If there is no event available, such as in Portal onMount, pass null.

Use data instead of props

There are several components which pass back information that is not included in props, such as an index or a state value. There may be props that we do not want to pass back at certain times as well.

I propose we then change the name of the props callback param to data and pass back as much props and state as we can that makes sense.

Reverse Parameter Order? (feedback please!)

NOTE: As per further discussion below, we decided to keep the order as-is (e, data). Keeping this here for posterity.

Given the above, I think it makes sense to reverse the callback parameter order from (e, data) to (data, e). My reasoning is as follows:

1. data covers the majority of use cases.
2. data should be preferred over event when they both contain the same information (ie value).
3. event is only provided as an escape hatch for edge cases, such as focus.
4. event is sometimes null, making it applicable in fewer cases.
5. This will already be a breaking change, which creates an opportunity for this kind of update.

New Documentation

Current

Callback signatures are inconsistently documented in doc blocks:

/** Called with (event, props) after user's click. */
onClick: PropTypes.func,

Updated

Use complete jsdoc tags to document functions. We will parse these into proper documentation.

/**
 * Called after user's click.
 * @param {SyntheticEvent} event - React's original SyntheticEvent.
 * @param {object} data - Relevant data.
 */
onClick: PropTypes.func,

Note, going forward this should apply to _all_ function props. We'll update the others in a separate effort.

TODO

For each component, update its code, tests, docs, and internal usages.

Update Callback Signatures

These components have non conformant callback signatures:

• [x] Accordion, AccordionTitle
• [x] BreadcrumbSection
• [x] Card
• [x] Checkbox
• [x] Confirm
• [x] Dropdown
• [x] Message
• [x] Menu, MenuItem
• [x] Search, SearchResult
• [x] Step
• [x] #920 TextArea
• [x] Rating, RatingIcon

Update Callback Docs

These components have non conformant function propType doc blocks:

• [x] Accordion, AccordionTitle
• [x] BreadcrumbSection
• [x] Card
• [x] Checkbox
• [x] Confirm
• [x] Dimmer
• [x] Dropdown, DropdownItem
• [x] Embed
• [x] Input
• [x] Label
• [x] Menu, MenuItem
• [x] Message
• [x] Modal
• [x] Popup
• [x] Portal
• [x] Rating, RatingIcon
• [x] Search, SearchCategory, SearchResult
• [x] Step
• [x] #920 TextArea
• [x] Table

[jeffcarbs]

I think we should keep the order of the arguments as-is, mainly because it would create inconsistency between events we handle and events we don't. This would be the required usage with the argument order change:

handleClick(event)
handleChange(data, event)

<Input onClick={handleClick} onChange={handleChange} />

This feels weird to me.

We could get around this by handling all events and always calling handlers with (data, e), but I really don't think that's a good idea.

[levithomason]

I'm of the mind all signatures should be consistent. So given:

<Input onClick={handleClick} onChange={handleChange} />

Callbacks would be either:

handleClick(data, e)
handleChange(data, e)

// or 

handleClick(e, data)
handleChange(e, data)

My initial thought is that the data first argument is the most common callback pattern and most likely to be used. My afterthought is that you are correct in that data may not make as much sense for some components. I hadn't considered a callback where we _would not_ pass back data. Though, I think this is a rare exception as even most on*Click handlers have data that needs to come back.

This gives me another idea, we have a single object that included the event:

handleChange({ event, ...data })
// e.g { event: [SyntheticEvent], value: 'hello' }

[jeffcarbs]

I'm of the mind all signatures should be consistent.

I 100% agree signatures should be consistent.

I hadn't considered a callback where we would not pass back data. Though, I think this is a rare exception as even most on*Click handlers have data that needs to come back.

I'm not actually talking about callbacks where _we (SUI-React)_ wouldn't pass back data. I'm talking about all the events that we don't explicitly handle for a given component. For example, the only event-related prop Input currently defines is onChange. This means that if I were to pass onClick, it would be an unhandled prop and so associated with the element with vanilla React.

So if we change the arguments, when the Input...

• is clicked, React will call onClick(syntheticEvent) as the argument
• receives a change, we intercept change and call the onChange(data, syntheticEvent)

This means I would need different signatures for each callback depending on if it's passed through to React or not.

With the other idea you mentioned ({ event, ...data }), the signatures would still be different for the two handlers:

handleClick(event) {
  const { target } = event 
}

handleChange({ event })
  const { target } = event
} 

I could see possibly spreading data _into_ the event:

// Input.js
onChange({ ...event, this.props })

// handler
handleChange(event) {
  event.preventDefault()

  const { name, value } = event
}

Although tbh I think is the best option is (event, data). It keeps us 100% consistent with the vanilla react API by passing the plain synthetic event as the first argument and returns additional useful data as a second argument where relevant.

@layershifter - would love your thoughts here as well 😄

[levithomason]

I see. The only way around that would be to intercept all events and rearg them, which is a bit ridiculous.

I'll update the spec, removing the reorder proposal.

We'll have 100% consistency in the signatures, though the presence of the arguments themselves will be inconsistent. We may want to consider using undefined when there is no event then. That way we at least limit the two argument positions to consistent values in all cases, they'll either be an object or undefined rather than an object, null, or undefined.

[levithomason]

I've updated the spec, removing reversed args, we'll call that the final spec. 👍

[levithomason]

There are actually still some components open on that list. I'll reopen this, update those, and close it with a new PR.

[craig1123]

Going back to my initial question before this is closed, is there an onMouseEnterOption prop (or some event handler for hovering over options) for the dropdown?

[levithomason]

@craig1123 you should be able to pass any valid React prop as an object key/value pair to the options. Each option object is used as a props object to each Dropdown.Item.