Skip to content
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

Add types for EuiEmptyPrompt, EuiCode(Block), EuiCallOut #1010

Merged
merged 8 commits into from
Jul 13, 2018
Merged

Add types for EuiEmptyPrompt, EuiCode(Block), EuiCallOut #1010

merged 8 commits into from
Jul 13, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 13, 2018

This PR adds type definitions for the <EuiEmptyPrompt>, <EuiCode>, <EuiCodeBlock>, and <EuiCallOut>. All components are pretty straight forward.

type Size = 's' | 'm';

export interface EuiCallOutProps {
title?: ReactNode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the title prop collides with the title attribute from HTMLAttributes<HTMLDivElement>, so TypeScript seems to be requiring that values in that prop meet both ReactNode and string which is weird... Experimenting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I just removed HTMLAttributes<HTMLDivElement> from the prop types signature for now, we can always add specific properties down the road if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can keep using HTMLAttributes and use Omit (defined in common.d.ts) to omit the original title prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I figured there must be a way to do that, couldn't figure it out.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM (all the props are there), but I defer to @chandlerprall when it comes to TypeScript.

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
- Make some proprties of `EuiFlyout` optional ([#1003](https://github.com/elastic/eui/pull/1003))
- Add typings for `EuiFlyout`, `EuiFlyoutBody`, `EuiFlyoutHeader`, and `EuiFlyoutFooter` ([#1001](https://github.com/elastic/eui/pull/1001))
- Gave `EuiFlyout` close button a data-test-subj ([#1000](https://github.com/elastic/eui/pull/1000))
- Add typings for `EuiEmptyPrompt`, `EuiCode`, `EuiCodeBlock`, and `EuiCallOut` ([#1010](https://github.com/elastic/eui/pull/1010))
Copy link
Contributor

Choose a reason for hiding this comment

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

Be sure to use past-tense in the changelog.

iconType?: IconType;
iconColor?: IconColor;
title?: ReactNode;
titleSize?: EuiTitleSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, eventually it looks like we'll need to align our naming. Some of these are prefixed and some are not.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM!

@spalger spalger merged commit cde4c1b into elastic:master Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants