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

Added missing union type for EuiPageContent #2516

Merged
merged 3 commits into from
Nov 13, 2019
Merged

Added missing union type for EuiPageContent #2516

merged 3 commits into from
Nov 13, 2019

Conversation

Drakota
Copy link
Contributor

@Drakota Drakota commented Nov 11, 2019

Summary

I added the HTMLAttributes<HTMLDivElement> type to EuiPageContent, because it seemed to be missing.

I've noticed this because I tried to use the style prop, but it wasn't available and I had to //@ts-ignore the component.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@Drakota
Copy link
Contributor Author

Drakota commented Nov 11, 2019

CLA signed 😊

@chandlerprall chandlerprall self-requested a review November 11, 2019 16:36
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.

Good catch, and thanks for the PR!

EuiPanel forwards the ...rest onto either a button or div element depending on if the onClick prop is present, which makes this typing incomplete. The distinction in props exists in EuiPanel line 55, but that definition isn't exported. Instead of adding the HTMDivElement props in page/index.d.ts it should export the full types from EuiPanel.

To sum it up, a better change is to rename the export at EuiPanel:55 to EuiPanelProps, and the existing EuiPanelProps at EuiPanel:15 to the generic Props, while removing that export.

@Drakota
Copy link
Contributor Author

Drakota commented Nov 12, 2019

Hey @chandlergibb,

I understand what you're saying and updated my pull request to reflect the necessary changes. 😃

Feel free to tell me if anything is missing.

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.

Changes LGTM, pulled to build & test the typescript changes locally, tested the generated eui.d.ts in Kibana and didn't see any issues.

@chandlerprall
Copy link
Contributor

@Drakota one more request: an entry to CHANGELOG.md's top master section with a quick description of this bug fix, something like "Fixed EuiPanelProps type definition"

@chandlerprall
Copy link
Contributor

jenkins test this

@chandlerprall
Copy link
Contributor

jenkins test this

@Drakota
Copy link
Contributor Author

Drakota commented Nov 13, 2019

I've added the CHANGELOG entry.
The Jenkins build failing seemed to be from the file not being prettified so I've fixed that as well.

@chandlerprall chandlerprall merged commit bd0875e into elastic:master Nov 13, 2019
@chandlerprall
Copy link
Contributor

Thanks again, @Drakota !

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