Skip to content

Convert EuiIcon to TypeScript#1355

Merged
pugnascotia merged 7 commits intoelastic:masterfrom
pugnascotia:convert-icon-to-ts
Dec 9, 2018
Merged

Convert EuiIcon to TypeScript#1355
pugnascotia merged 7 commits intoelastic:masterfrom
pugnascotia:convert-icon-to-ts

Conversation

@pugnascotia
Copy link
Contributor

Summary

Convert the EuiIcon component to TypeScript. Notes:

  • I switched the invocation of tsc and tslint, as TSLint recommends that compilation run cleanly before linting.
  • I added a directory for custom library definitions, so that TS wouldn't complain about importing SVGs. These imports are still compiled into inline React components with Babel.
  • I removed the icon title tests, since they have been deprecated and wrong for a long time.

Something else to note is that I'm seeing the following warning when building EUI now:

[BABEL] Note: The code generator has deoptimised the styling of /Users/rory/src/elastic/eui/src/components/icon/icon.tsx as it exceeds the max of 500KB.

True enough, if you look at the output in lib/, it's...ugly. I ran it through Prettier, and did the same for master, and the output varies in the expected ways but not by that much. Perhaps I just inched over the threshold? I did wonder if we should stop using the plugin and write a script to call svgo and generate actual component files instead? In any case, I started the docs site locally and the icons looked fine.

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

if (COLORS.indexOf(color) > -1) {
optionalColorClass = colorToClassMap[color];
if (color) {
checkValidColor(color, type || 'empty');
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting way to re-implement this prop type, I like it!

@chandlerprall
Copy link
Contributor

That babel warning is fine. It's on our roadmap to begin addressing EuiIcon's overall size.

@pugnascotia
Copy link
Contributor Author

@chandlerprall I've pushed some review feedback, but the prop types generator fails on these again. Might be the keysOf change - would you mind taking a look?

@pugnascotia
Copy link
Contributor Author

Cancel that - everything's good after rebasing, so I've also inlined that interface that I mentioned above. 🙌

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.

LGTM, nice work!

@pugnascotia pugnascotia merged commit 244a779 into elastic:master Dec 9, 2018
@pugnascotia pugnascotia deleted the convert-icon-to-ts branch December 9, 2018 12:29
@snide snide mentioned this pull request Dec 11, 2018
3 tasks
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