Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS by pugnascotia · Pull Request #2391 · elastic/eui

pugnascotia · 2019-10-01T09:38:54Z

Summary

Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TypeScript. I had to resort to some type casts around passing props to EuiButtonEmpty, so if anyone has any better ideas, please say.

Checklist

Checked in dark mode
Checked in mobile
Checked in IE11 and Firefox
Props have proper autodocs
Added documentation examples
Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

thompsongl

Nobody beats Rory's branch names

thompsongl · 2019-10-01T22:00:37Z

src/components/breadcrumbs/breadcrumbs.tsx


-const limitBreadcrumbs = (breadcrumbs, max, showMaxPopover, allBreadcrumbs) => {
+export type Breadcrumb = CommonProps & {
+  text: string;


This previously accepted node, so we probably want to have ReactNode here.

The issue here (and I meant to mention it in the PR) is that text is also used in a title attribute, so a <span> for example is rendered as title="[object Object]", which you could see being changed in one of the snapshots.

Ah right. We have a utility that'll help with this. Let me look into it

Ok. Updated.

thompsongl · 2019-10-01T22:03:03Z

src/components/breadcrumbs/index.ts

@@ -0,0 +1 @@
+export { Breadcrumb, EuiBreadcrumbs } from './breadcrumbs';


Let's export EuiBreadcrumbsProps also

thompsongl · 2019-10-01T22:04:56Z

src/components/header/header.tsx

+
+import { CommonProps } from '../common';
+
+export const EuiHeader: FunctionComponent<CommonProps> = ({


Need to add HTMLAttributes<HTMLDivElement> since we eventually spread props onto the div

thompsongl · 2019-10-01T22:06:00Z

src/components/header/header_alert/header_alert.tsx

 import { EuiI18n } from '../../i18n';

-export const EuiHeaderAlert = ({
+type EuiHeaderAlertProps = CommonProps & {


Again, need to add HTMLAttributes<HTMLDivElement> since we eventually spread props onto the div (L31)

thompsongl · 2019-10-01T22:08:33Z

src/components/header/header_section/header_section_item_button.tsx

-export const EuiHeaderSectionItemButton = ({
+import { CommonProps } from '../../common';
+
+type Props = CommonProps & AnchorHTMLAttributes<HTMLButtonElement>;


Suggested change

type Props = CommonProps & AnchorHTMLAttributes<HTMLButtonElement>;

type Props = CommonProps & ButtonHTMLAttributes<HTMLButtonElement>;

thompsongl · 2019-10-01T22:10:18Z

src/components/header/header_logo.tsx

-  children?: ReactNode;
-}
+export type EuiHeaderLogoProps = CommonProps &
+  HTMLAttributes<HTMLAnchorElement> & {


Suggested change

HTMLAttributes<HTMLAnchorElement> & {

AnchorHTMLAttributes<HTMLAnchorElement> & {

thompsongl · 2019-10-01T22:11:32Z

src/components/header/header_links/header_link.tsx

+import { IconType } from '../../icon';
+
+type Props = CommonProps &
+  AnchorHTMLAttributes<HTMLAnchorElement> & {


Looks like this could extend EuiButtonEmptyProps instead? Wouldn't have to cast it later on (I think)

Cast is still needed, unfortunately.

thompsongl · 2019-10-01T22:13:09Z

src/components/header/header_breadcrumbs/header_breadcrumbs.tsx

+import { CommonProps } from '../../common';
+import { Breadcrumb, EuiBreadcrumbs } from '../../breadcrumbs';
+
+type Props = CommonProps & {


Extend EuiBreadCrumbProps also since this spreads rest later

pugnascotia · 2019-10-02T15:36:42Z

Cool, thanks @thompsongl. Any chance of a LGTM then?

thompsongl

LGTM after changelog update.

Tested eui.d.ts file in Kibana and have a local branch with necessary updates for when the time comes.

Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TypeScript.

Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS

937ccc3

pugnascotia added the typescript label Oct 1, 2019

pugnascotia requested review from chandlerprall and thompsongl October 1, 2019 09:38

thompsongl suggested changes Oct 1, 2019

View reviewed changes

pugnascotia and others added 2 commits October 2, 2019 11:53

Address review feedback

ff184e5

allow ReactNode as breadcrumb children

4cd8065

euilink interface exports

ab83764

thompsongl approved these changes Oct 2, 2019

View reviewed changes

Merge branch 'master' into raiders-of-the-lost-ts

d1093f3

pugnascotia changed the title ~~Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS~~ EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS Oct 4, 2019

pugnascotia merged commit f829b0e into elastic:master Oct 4, 2019

pugnascotia deleted the raiders-of-the-lost-ts branch October 4, 2019 11:29

pugnascotia changed the title ~~EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS~~ Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS Oct 4, 2019

snide pushed a commit to snide/eui that referenced this pull request Oct 10, 2019

EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS (elastic#2391)

9d94ba8

Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TypeScript.

thompsongl mentioned this pull request Oct 14, 2019

Upgrade EUI to 14.5.0 elastic/kibana#48119

Merged

		@@ -0,0 +1 @@
		export { Breadcrumb, EuiBreadcrumbs } from './breadcrumbs';


		import { CommonProps } from '../common';

		export const EuiHeader: FunctionComponent<CommonProps> = ({

	type Props = CommonProps & AnchorHTMLAttributes<HTMLButtonElement>;
	type Props = CommonProps & ButtonHTMLAttributes<HTMLButtonElement>;

	HTMLAttributes<HTMLAnchorElement> & {
	AnchorHTMLAttributes<HTMLAnchorElement> & {

Conversation

pugnascotia commented Oct 1, 2019

Summary

Checklist

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia commented Oct 2, 2019

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants