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 services to TypeScript, part 1 #1360

Merged
merged 14 commits into from
Dec 12, 2018

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Dec 9, 2018

Summary

This PR converts roughly half the services to TypeScript. Implementation notes:

  • In browser.ts, I tried to make it clear what contract Browser was fulfilling, and I may have ended up with something more verbose that necessary.
  • color_palette.ts had its own hex -> RGB convertor, so I replaced it with hexToRgb. I also removed formatHex() since it was redundant.
  • In rgb_to_hex.ts, I replaced the single ugly regex with two passes to simplify the implementation. I could still do it in one, and maybe it would be more legible than before? Opinions welcome. I also made it stricter, since the original didn't check the whole argument value i.e, you could pass rgb(1,2,3 and it would return a hex code.
  • In random.ts, I ran up against Wrong 'this' context in class methods microsoft/TypeScript#3927 and had to make all the method definitions bound.

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 pugnascotia changed the title WIP - Convert services to TypeScript, part 1 Convert services to TypeScript, part 1 Dec 10, 2018
@pugnascotia
Copy link
Contributor Author

I've added the tests that I intended, so this is no longer WIP.

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.

Couple small things & questions. Looks fantastic though.

src/services/accessibility/html_id_generator.ts Outdated Show resolved Hide resolved
src/services/color/color_palette.ts Outdated Show resolved Hide resolved
src/services/color/rgb_to_hex.ts Outdated Show resolved Hide resolved
src/services/random.ts Show resolved Hide resolved
@pugnascotia
Copy link
Contributor Author

@chandlerprall I've addressed your feedback and rebased on master.

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.

LGreatTM !

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