EuiTab href #1
EuiTab href #1
Conversation
| @@ -37,4 +39,5 @@ declare module '@elastic/eui' { | |||
| export const EuiTabbedContent: FunctionComponent< | |||
| EuiTabbedContentProps & CommonProps & HTMLAttributes<HTMLDivElement> | |||
| >; | |||
| export const EuiTab: FunctionComponent<EuiTabProps>; | |||
There was a problem hiding this comment.
Because the tabs/ directory isn't entirely TypeScript yet, we still need to export all the components from here so they can be picked up properly when imported.
Once all of tabs/ has been converted, this file will be removed.
| @@ -27,13 +26,13 @@ describe('EuiTab', () => { | |||
| describe('Props', () => { | |||
| describe('onClick', () => { | |||
| test('is called when the button is clicked', () => { | |||
| const onClickHandler = sinon.stub(); | |||
| const onClickHandler = jest.fn(); | |||
There was a problem hiding this comment.
When converting a component to TS, also convert the related test file.
We don't use the sinon library much, so I just replaced it with a parallel library that we do use.
| @@ -17,25 +16,21 @@ export interface EuiTabProps extends CommonProps { | |||
| type EuiTabPropsForAnchor = EuiTabProps & | |||
| AnchorHTMLAttributes<HTMLAnchorElement> & { | |||
| href?: string; | |||
| onClick?: MouseEventHandler<HTMLButtonElement>; | |||
| buttonRef?: Ref<HTMLAnchorElement>; | |||
| onClick?: MouseEventHandler<HTMLAnchorElement>; | |||
There was a problem hiding this comment.
HTMLAnchorElement for the <a>
| @@ -17,25 +16,21 @@ export interface EuiTabProps extends CommonProps { | |||
| type EuiTabPropsForAnchor = EuiTabProps & | |||
| AnchorHTMLAttributes<HTMLAnchorElement> & { | |||
| href?: string; | |||
| onClick?: MouseEventHandler<HTMLButtonElement>; | |||
| buttonRef?: Ref<HTMLAnchorElement>; | |||
There was a problem hiding this comment.
I think this was a leftover from a copy+paste. The component didn't use ref before, so we can just delete this part.
| @@ -55,12 +50,10 @@ export const EuiTab: FunctionComponent<Props> = ({ | |||
| <a | |||
| role="tab" | |||
| aria-selected={!!isSelected} | |||
| onClick={onClick as MouseEventHandler<HTMLAnchorElement>} | |||
There was a problem hiding this comment.
Just a clean up to avoid the ominous as casting. onClick is now part of ...rest, which is more of a subjective choice, but it's more in line with how we've handled other button-like components.
|
Thanks @thompsongl. I was reviewing the comments and they make sense. |
* removed duplicate icons * Fixing changelog and updating tests (#1) Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Just a little PR to help you learn what all goes into a TS conversion PR.
I'll leave comments in each file to describe what's what. I'm more than happy to walk you through anything that doesn't make sense on Zoom