Skip to content

Add basic typings for Flyout#1001

Merged
stacey-gammon merged 4 commits into
elastic:masterfrom
stacey-gammon:2018-07-11-typescript-flyout
Jul 11, 2018
Merged

Add basic typings for Flyout#1001
stacey-gammon merged 4 commits into
elastic:masterfrom
stacey-gammon:2018-07-11-typescript-flyout

Conversation

@stacey-gammon
Copy link
Copy Markdown

@stacey-gammon stacey-gammon commented Jul 11, 2018

Comment thread src/components/flyout/index.d.ts Outdated
@@ -0,0 +1,3 @@
declare module '@elastic/eui' {
export const EuiFlyout: React.SFC<any>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we put it into EUI, should we rather also properly type the Properties here?

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.

ohhhh alright alright. :) Typings expanded.

@stacey-gammon stacey-gammon force-pushed the 2018-07-11-typescript-flyout branch from 9c17f8d to 7ef218d Compare July 11, 2018 17:37
@cchaos cchaos requested a review from chandlerprall July 11, 2018 18:12
Copy link
Copy Markdown
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.

LGreatTM!

@stacey-gammon stacey-gammon force-pushed the 2018-07-11-typescript-flyout branch from 587dd27 to 2d03f5e Compare July 11, 2018 19:14
@stacey-gammon stacey-gammon merged commit fbd8602 into elastic:master Jul 11, 2018
declare module '@elastic/eui' {
export interface EuiFlyoutProps {
onClose: () => void;
size: 's' | 'm' | 'l';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that typing might be wrong. Is size really a mandatory property? If not it should be marked as optional size? in the property interface.

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.

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.

3 participants