Skip to content

EuiTab href#1

Merged
elizabetdev merged 1 commit intoelizabetdev:euitab-href-propfrom
thompsongl:euitab-href-prop
Sep 4, 2019
Merged

EuiTab href#1
elizabetdev merged 1 commit intoelizabetdev:euitab-href-propfrom
thompsongl:euitab-href-prop

Conversation

@thompsongl
Copy link
Copy Markdown

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

export const EuiTabbedContent: FunctionComponent<
EuiTabbedContentProps & CommonProps & HTMLAttributes<HTMLDivElement>
>;
export const EuiTab: FunctionComponent<EuiTabProps>;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

describe('onClick', () => {
test('is called when the button is clicked', () => {
const onClickHandler = sinon.stub();
const onClickHandler = jest.fn();
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

href?: string;
onClick?: MouseEventHandler<HTMLButtonElement>;
buttonRef?: Ref<HTMLAnchorElement>;
onClick?: MouseEventHandler<HTMLAnchorElement>;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

HTMLAnchorElement for the <a>

AnchorHTMLAttributes<HTMLAnchorElement> & {
href?: string;
onClick?: MouseEventHandler<HTMLButtonElement>;
buttonRef?: Ref<HTMLAnchorElement>;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think this was a leftover from a copy+paste. The component didn't use ref before, so we can just delete this part.

<a
role="tab"
aria-selected={!!isSelected}
onClick={onClick as MouseEventHandler<HTMLAnchorElement>}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@elizabetdev
Copy link
Copy Markdown
Owner

Thanks @thompsongl. I was reviewing the comments and they make sense.

@elizabetdev elizabetdev merged commit 444d3aa into elizabetdev:euitab-href-prop Sep 4, 2019
elizabetdev pushed a commit that referenced this pull request Mar 5, 2020
* removed duplicate icons

* Fixing changelog and updating tests (#1)

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
elizabetdev pushed a commit that referenced this pull request Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants