Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range TS #2349

Merged
merged 4 commits into from
Sep 17, 2019
Merged

Range TS #2349

merged 4 commits into from
Sep 17, 2019

Conversation

maryia-lapata
Copy link
Contributor

Summary

This PR updates TS def for onChange method of EuiRange.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -62,7 +62,9 @@ declare module '@elastic/eui' {
}

export const EuiRange: FunctionComponent<
CommonProps & InputHTMLAttributes<HTMLInputElement> & EuiRangeProps
CommonProps &
Omit<InputHTMLAttributes<HTMLInputElement>, 'onChange'> &
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case if we don't omit onChange def from InputHTMLAttributes<HTMLInputElement>, EuiRange's onChange requires ((event: React.ChangeEvent<HTMLInputElement>) => void) & ((event: React.ChangeEvent<HTMLInputElement> | React.MouseEvent<HTMLButtonElement, MouseEvent>, isValid: boolean) => void)) def.

@maryia-lapata
Copy link
Contributor Author

@chandlerprall getting back to the comment #2211 (comment), React.MouseEvent<HTMLButtonElement> limits target and if we want to use target's valueAsNumber the event should be cast to React.ChangeEvent<HTMLInputElement> (e.g. https://github.com/elastic/kibana/pull/45902/files#diff-e08b038e0994227720936e08475c5502R57). Maybe you have some idea how to avoid casting?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, created an empty typescript project locally, added EUI as a dependency, and verified the bug before this change and that it is fixed with this it.

Needs a bugfix entry to CHANGELOG.md before merging

@chandlerprall
Copy link
Contributor

jenkins test this

@chandlerprall
Copy link
Contributor

You can access valueAsNumber (TIL about this attribute!) without casting by type checking first -

if (e.currentTarget instanceof HTMLInputElement) {
    ... do something with e.currentTarget.valueAsNumber ...
}

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Tested locally by consuming change in an new TS project

@chandlerprall chandlerprall merged commit 628a888 into elastic:master Sep 17, 2019
@maryia-lapata maryia-lapata deleted the range-onchange branch September 17, 2019 16:08
thompsongl added a commit that referenced this pull request Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants