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 badge and token components to TypeScript #2026

Merged
merged 9 commits into from
Jun 11, 2019

Conversation

pugnascotia
Copy link
Contributor

Summary

Convert the badge and token components to TypeScript.

Note that EuiBadge was using prop type validation to check that ARIA labels were being supplied along with other props. Having converted the component to TS, this check now throws an error when it fails, with the intent being to catch this problem during development. Let me know if you think this is over-zealous (as I half-feel it might be).

The same is true for EuiBetaBadge when used without a tooltip - since in this case a title attribute is added to the <span> that is rendered, the attribute value must be a string. The component now forcefully checks this.

I also ended up converting the prop type utils. I'm less certain of what I've done here, so feedback welcome.

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

@pugnascotia
Copy link
Contributor Author

jenkins test this

@chandlerprall chandlerprall self-assigned this Jun 11, 2019
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.

More structured TS definitions for the props; should be able to change the errors to console.warn to better mimic proptypes when the consuming application isn't using Typescript.

src/components/badge/beta_badge/beta_badge.tsx Outdated Show resolved Hide resolved
src/components/badge/beta_badge/beta_badge.tsx Outdated Show resolved Hide resolved
src/components/badge/badge.tsx Outdated Show resolved Hide resolved
src/components/badge/badge.tsx Outdated Show resolved Hide resolved
src/components/badge/badge.tsx Outdated Show resolved Hide resolved
src/components/badge/badge.tsx Outdated Show resolved Hide resolved
src/components/badge/badge.tsx Outdated Show resolved Hide resolved
src/utils/prop_types/with_required_prop.ts Outdated Show resolved Hide resolved
HTMLAttributes<HTMLSpanElement> &
BadgeProps;

export const EuiBetaBadge: FunctionComponent<EuiBetaBadeProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, EuiBetaBadeProps -> EuiBetaBadgeProps

CHANGELOG.md Outdated
@@ -4,6 +4,9 @@
- Convert observer utility components to TypeScript ([#2009](https://github.com/elastic/eui/pull/2009))
- Convert tool tip components to TypeScript ([#2013](https://github.com/elastic/eui/pull/2013))

**Breaking changes**
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the breaking changes label

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; built locally and confirmed eui.d.ts is as expected

@pugnascotia pugnascotia merged commit ab01b1b into elastic:master Jun 11, 2019
@pugnascotia pugnascotia deleted the yet-more-ts-migrations branch June 11, 2019 21:56
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.

2 participants