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

Convert EuiCopy component to TypeScript #2016

Merged
merged 7 commits into from
Jun 11, 2019

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Jun 10, 2019

Summary

Convert EuiCopy component to TypeScript.

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@scottybollinger scottybollinger changed the title Add TypeScript definitions to Copy component Convert EuiCopy component to TypeScript Jun 10, 2019
@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2019

Thanks @scottybollinger !

As mentioned in Slack, we are in the process of actually converting the components to true TS instead of adding the TS defs as a shim. We can absolutely proceed with reviewing this PR as is unless you want to take a stab at converting to TS?

@scottybollinger
Copy link
Contributor Author

@cchaos I went ahead and converted it and updated the title and description of the PR. Let me know if you need anything else!

src/components/copy/copy.tsx Outdated Show resolved Hide resolved
src/components/copy/copy.tsx Outdated Show resolved Hide resolved
src/components/copy/copy.tsx Outdated Show resolved Hide resolved
src/components/copy/copy.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
/**
* Tooltip message displayed before copy function is called.
*/
beforeMessage: ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

this prop is optional, please update the interface to match: beforeMessage?: 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.

Thanks @chandlerprall. Just to be sure, is there an associated defaultProps value for this as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there is not (thanks for double checking!) - it's allowed to be null and in that case nothing is rendered.

src/components/copy/copy.tsx Show resolved Hide resolved
src/components/copy/copy.tsx Outdated Show resolved Hide resolved
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.

Thanks, LGTM!

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

@scottybollinger scottybollinger merged commit 382c850 into elastic:master Jun 11, 2019
@scottybollinger scottybollinger deleted the copy-typescript-defs branch June 11, 2019 19:15
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.

4 participants