Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(typings): synthetic event#740

Merged
kuzhelov merged 3 commits intomasterfrom
fix/synthetic-event-type
Jan 17, 2019
Merged

fix(typings): synthetic event#740
kuzhelov merged 3 commits intomasterfrom
fix/synthetic-event-type

Conversation

@kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Jan 17, 2019

This PR explicitly specifies generic param (as HTMLElement) of type that is used for synthetic event arg in component's event handlers.

This is necessary for client libraries to avoid explicit cast from Element (currently provided by Stardust) to HTMLElement which is expected by client's code.

TODO

  • add changelog entry

export type ReactProps<T> = Extendable<ReactPropsStrict<T>>

export type ComponentEventHandler<TProps> = (event: React.SyntheticEvent, data: TProps) => void
export type ComponentEventHandler<TProps> = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type ComponentEventHandler<TProps> = (
export type ComponentEventHandler<TProps, TElem = HTMLElement> = (

Can we make this a generic?

Copy link
Contributor Author

@kuzhelov kuzhelov Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, although not sure whether it is necessary to introduce this variability now, as it is pretty much clear that it will always be about HTMLElement for this handler's type - thus, not sure if we should make this type to be customizable (as it will, probably, will be an indication of some improper assumptions)

@kuzhelov kuzhelov merged commit 11614e0 into master Jan 17, 2019
@kuzhelov kuzhelov deleted the fix/synthetic-event-type branch January 17, 2019 12:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants