-
Notifications
You must be signed in to change notification settings - Fork 842
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
[types/EuiButtonEmpty] add missing isLoading and href props #992
Conversation
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.
Looks right to me. Would you mind taking it a bit further and adding those props to the other 2 button types as well?
@chandlerprall Any idea why onClick
would not have been added to EuiButtonEmptyProps
originally and if we should go ahead and add it now?
@spalger Also, be sure to add a changelog entry in CHANGELOG.md (you should just be able to follow the pattern).
@cchaos |
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.
Needs bugfix entry to CHANGELOG.md
@chandlerprall thanks, updated the changelog, removed the requirement for |
So, the latest types I supplied are somewhat undesirable because it changes the type of the |
Okay, these types work for the following situations: <EuiButton
href="string"
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => undefined}
/> <EuiButton
onClick={(e: React.MouseEvent<HTMLButtonElement>) => undefined}
/> <EuiButton
href={optionalHref}
onClick={(e: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>) => undefined}
/> |
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.
Changes LGTM. Nice solution @spalger !
Tried to use
<EuiButtonEmpty />
in typescript but thehref
prop wasn't typed, pulled over theisLoading
prop too because it seemed to be missing but is defined in thepropTypes