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 EuiText, EuiTextColor and EuiTextAlign to TS #1791

Merged
merged 6 commits into from
Apr 16, 2019

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Apr 3, 2019

Summary

Convert EuiText, EuiTextColor and EuiTextAlign 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

@chandlerprall
Copy link
Contributor

TS certainly doesn't make it easy to track down some error's root locations!

The issue is EuiSelectableMessageProps has HTMLAttributes<HTMLDivElement>, and then spreading rest onto EuiText. The color prop is sneaking through and, if present in rest, overrides color="subdued". You can either omit color from HTMLAttributes<HTMLDivElement> or apply the color prop after rest:

<EuiText size="xs" className={classes} {...rest} color="subdued">

This is an irk of mine where the React API works opposite of how we talk about props:

"EuiText takes the important things like size, color, and className, and then any extra props we don't care about"

whereas the API gives a higher priority to props that appear later, which matches how JS objects work:

{a:1, a:2} // {a:2}

It is more correct to place the rest spread at the beginning of the props in most cases because of this, but that is non-unintuitive and rarely matters.

@pugnascotia pugnascotia changed the title [WIP] Convert EuiText, EuiTextColor and EuiTextAlign to TS Convert EuiText, EuiTextColor and EuiTextAlign to TS Apr 15, 2019
@pugnascotia
Copy link
Contributor Author

@chandlerprall thanks for the tip, this is ready to review now.

@@ -0,0 +1,5 @@
export { EuiText, TextSize, TEXT_SIZES } from './text';
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should only export the components (match the same export signature as the removed index.js and index.d.ts, both of which only export the components). The other values shouldn't be used/usable outside of EUI, and internally other components an require them directly from their source files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chandlerprall I've changed the index to only export the components.

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!

@chandlerprall
Copy link
Contributor

jenkins test this

@pugnascotia pugnascotia merged commit 2b71d39 into elastic:master Apr 16, 2019
@pugnascotia pugnascotia deleted the convert-text-to-ts branch April 16, 2019 15:36
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