-
Notifications
You must be signed in to change notification settings - Fork 829
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
Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS #2391
Migrate EuiBreadcrumbs, EuiHeader and children, and EuiLink to TS #2391
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.
Nobody beats Rory's branch names
import { EuiBadge } from '../badge'; | ||
import { EuiI18n } from '../i18n'; | ||
import { EuiLink } from '../link'; | ||
import { EuiPopover } from '../popover'; | ||
|
||
const limitBreadcrumbs = (breadcrumbs, max, showMaxPopover, allBreadcrumbs) => { | ||
export type Breadcrumb = CommonProps & { | ||
text: string; |
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.
This previously accepted node
, so we probably want to have ReactNode
here.
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.
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.
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.
Ah right. We have a utility that'll help with this. Let me look into it
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.
Ok. Updated.
src/components/breadcrumbs/index.ts
Outdated
@@ -0,0 +1 @@ | |||
export { Breadcrumb, EuiBreadcrumbs } from './breadcrumbs'; |
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.
Let's export EuiBreadcrumbsProps
also
src/components/header/header.tsx
Outdated
|
||
import { CommonProps } from '../common'; | ||
|
||
export const EuiHeader: FunctionComponent<CommonProps> = ({ |
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.
Need to add HTMLAttributes<HTMLDivElement>
since we eventually spread props onto the div
|
||
import { EuiFlexGroup, EuiFlexItem } from '../../flex'; | ||
|
||
import { EuiI18n } from '../../i18n'; | ||
|
||
export const EuiHeaderAlert = ({ | ||
type EuiHeaderAlertProps = CommonProps & { |
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.
Again, need to add HTMLAttributes<HTMLDivElement>
since we eventually spread props onto the div
(L31)
export const EuiHeaderSectionItemButton = ({ | ||
import { CommonProps } from '../../common'; | ||
|
||
type Props = CommonProps & AnchorHTMLAttributes<HTMLButtonElement>; |
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.
type Props = CommonProps & AnchorHTMLAttributes<HTMLButtonElement>; | |
type Props = CommonProps & ButtonHTMLAttributes<HTMLButtonElement>; |
children?: ReactNode; | ||
} | ||
export type EuiHeaderLogoProps = CommonProps & | ||
HTMLAttributes<HTMLAnchorElement> & { |
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.
HTMLAttributes<HTMLAnchorElement> & { | |
AnchorHTMLAttributes<HTMLAnchorElement> & { |
import { IconType } from '../../icon'; | ||
|
||
type Props = CommonProps & | ||
AnchorHTMLAttributes<HTMLAnchorElement> & { |
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 like this could extend EuiButtonEmptyProps
instead? Wouldn't have to cast it later on (I think)
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.
Cast is still needed, unfortunately.
import { CommonProps } from '../../common'; | ||
import { Breadcrumb, EuiBreadcrumbs } from '../../breadcrumbs'; | ||
|
||
type Props = CommonProps & { |
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.
Extend EuiBreadCrumbProps
also since this spreads rest
later
Cool, thanks @thompsongl. Any chance of a LGTM then? |
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.
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.
Summary
Migrate
EuiBreadcrumbs
,EuiHeader
and children, andEuiLink
to TypeScript. I had to resort to some type casts around passing props toEuiButtonEmpty
, so if anyone has any better ideas, please say.Checklist