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

Adds TypeScript support #545

Merged
merged 1 commit into from
Jul 17, 2018
Merged

Adds TypeScript support #545

merged 1 commit into from
Jul 17, 2018

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jul 13, 2018 •
edited
Loading

Resolves #559

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@NejcZdovc NejcZdovc self-assigned this Jul 13, 2018
@NejcZdovc NejcZdovc changed the title WIP: Adds TypeScript support Adds TypeScript support Jul 16, 2018
@NejcZdovc NejcZdovc requested a review from bbondy July 16, 2018 11:07
@bbondy bbondy requested review from cezaraugusto and yrliou July 16, 2018 14:34
yrliou
yrliou approved these changes Jul 16, 2018
View reviewed changes
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

++

cezaraugusto
cezaraugusto suggested changes Jul 17, 2018
View reviewed changes
package.json
"react": "^16.2.0",
"react-dnd": "^2.5.4",
"react-dnd-html5-backend": "^2.5.4",
"react-dnd": "^5.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

side note: for the future let's avoid bumping major versions together with PR proposal as it makes much harder to review as we need to manually test these functionalities too.

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi I'm facing this error after the update:

screen shot 2018-07-17 at 12 48 38 am

Copy link
Contributor

Choose a reason for hiding this comment

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

downgrading to current version fixed the issue.

since brave/brave-core#253 depends on this PR I'm blocking/requesting changes on this.

I follow react-dnd closely and see that it's rewritten in TS so If above PR somehow solves this let's bump this dependency in a follow-up. Otherwise lgtm

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I tested it out again and it's working for me. Maybe you have something cached. I bumped versions, because there were some problems with typing and compiling. Can you please try again?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok @yrliou just confirmed it's working for her too so it's something cached I'm seeing. I'm changing feedback/merging after. thanks

cezaraugusto
cezaraugusto approved these changes Jul 17, 2018
View reviewed changes
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

approving after #545 (comment)

@cezaraugusto cezaraugusto merged commit be15b73 into brave:master Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yrliou yrliou yrliou approved these changes

@cezaraugusto cezaraugusto cezaraugusto approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@NejcZdovc NejcZdovc

Labels
dev-setup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NejcZdovc @cezaraugusto @yrliou