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.

Sign up for GitHub

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

Jump to bottom

Add support for Typescript and update dependencies #87

Merged
merged 13 commits into from
Apr 19, 2022
Merged

Add support for Typescript and update dependencies #87

merged 13 commits into from
Apr 19, 2022

Conversation

thiteixeira
Copy link
Contributor

@thiteixeira thiteixeira commented Apr 9, 2022 •
edited
Loading

  • Add support for Typescript
  • Upgrade dependencies
  • Use React 18
  • Add enum for station names to make them more readable
  • Remove unused exports

@thiteixeira thiteixeira requested a review from idreyn April 10, 2022 02:07
@thiteixeira thiteixeira marked this pull request as ready for review April 10, 2022 02:07
nathan-weinberg
nathan-weinberg requested changes Apr 12, 2022
View reviewed changes
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

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

Not a JavaScript/TypeScript expert by any means so I'll leave more technical comments for others - at a layman's glance LGTM, just one typo needing fixing. Please ensure the changes are compatible with CI! (looks like they are)

package.json Outdated Show resolved Hide resolved
@nathan-weinberg nathan-weinberg self-requested a review April 13, 2022 22:51
nathan-weinberg
nathan-weinberg reviewed Apr 13, 2022
View reviewed changes
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

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

No further comments on my end

idreyn
idreyn approved these changes Apr 15, 2022
View reviewed changes
Copy link
Contributor

@idreyn idreyn left a comment

Choose a reason for hiding this comment

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

I have a few suggestions inline but overall this looks great and I'm excited to start using TypeScript in this codebase!

src/components/App.js Outdated Show resolved Hide resolved
src/lines.ts Outdated Show resolved Hide resolved
@thiteixeira thiteixeira merged commit 446cd64 into master Apr 19, 2022
@thiteixeira thiteixeira deleted the tteixeira_initial_pr branch April 19, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes

@idreyn idreyn idreyn approved these changes

@austinjpaul austinjpaul Awaiting requested review from austinjpaul

@friendchris friendchris Awaiting requested review from friendchris

@mathcolo mathcolo Awaiting requested review from mathcolo

@skaplan-dev skaplan-dev Awaiting requested review from skaplan-dev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thiteixeira @idreyn @nathan-weinberg