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

Use ES6 classes in ‘Motion.js’ #457

Merged
merged 9 commits into from
Apr 25, 2017
Merged

Use ES6 classes in ‘Motion.js’ #457

merged 9 commits into from
Apr 25, 2017

Conversation

ifndefdeadmau5
Copy link
Contributor

@ifndefdeadmau5 ifndefdeadmau5 commented Apr 25, 2017 •
edited
Loading

Hi,

npm install has broken and it seems previous migration PR missed to fix one file Motion.js
This PR is trying to fix this. I tested npm start on my local machine and worked well.
Thanks

@nkbt
Copy link
Collaborator

nkbt commented Apr 25, 2017

👍
I wonder how our tests did not fail

nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
package.json Outdated
@@ -73,6 +73,7 @@
"author": "chenglou",
"license": "MIT",
"dependencies": {
"create-react-class": "15.5.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm, probably not intentional

nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
src/Motion.js Outdated
@@ -20,8 +19,8 @@ type MotionState = {
lastIdealVelocity: Velocity,
};

const Motion = createReactClass({
propTypes: {
export default class Motion extends Component {
Copy link
Collaborator

Choose a reason for hiding this comment

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

All other files use export default class Demo extends React.Component way, it would be better to keep this one aligned too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

nkbt
nkbt requested changes Apr 25, 2017
View reviewed changes
Copy link
Collaborator

@nkbt nkbt left a comment

Choose a reason for hiding this comment

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

couple of nits

nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
src/Motion.js Outdated
@@ -110,7 +117,7 @@ const Motion = createReactClass({
return;
}

this.wasAnimating = true;
this.wasAnimating = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ouch

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is probably why tests fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to avoid flow check error(current error message on travis), but I guess it causes karma error... I'm not farmiliar with Flow and internal logic of this file, Could you give me an idea how to fix this flow error?

nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
src/Motion.js Outdated
animationID: (null: ?number),
prevTime: 0,
accumulatedTime: 0,
wasAnimating: false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should probably go to constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is why flowtype fails. wasAnimating and others are prototype properties, not instance properties. So as soon as you put them all to constructor like this.wasAnimating = false, tests will pass (I think so)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I put this.wasAnimating = false first, and it gets this error

src/Motion.js:120
120:       this.wasAnimating = true;
                ^^^^^^^^^^^^ property `wasAnimating`. Property not found in
 22: export default class Motion extends React.Component {
                          ^^^^^^ Motion

Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to update types to include it, I reckon.

I am not doing much of Flow and never used it with JS Classes, so cannot help much in this case. Maybe someone can give you better advice.

Or there must be some examples of using Flow with React Classes somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! that sounds quite promising, will research on that.

@nkbt nkbt mentioned this pull request Apr 25, 2017
Merged
nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
src/Motion.js Outdated
animationID: (null: ?number),
prevTime: 0,
accumulatedTime: 0,
animationID: ?number = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess all instance properties, not only wasAnimating...

Copy link
Contributor Author

@ifndefdeadmau5 ifndefdeadmau5 Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes :) I was just testing with one property to see if different travis result comes out compare to my local test.. now I moved them all

nkbt
nkbt reviewed Apr 25, 2017
View reviewed changes
src/Types.js Outdated
@@ -37,6 +37,10 @@ export type MotionProps = {
style: Style,
children: (interpolatedStyle: PlainStyle) => ReactElement,
onRest?: () => void,
wasAnimating?: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These properties are not MotionProps, they are instance properties of Motion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so sounds like I need to declare this

type Motion = {
  wasAnimating?: boolean,
  animationID?: number,
  prevTime: number,
  accumulatedTime: number,
}

in the Motion.js file itself?

@nkbt
Copy link
Collaborator

nkbt commented Apr 25, 2017

hm, here is info on how to use instance props.

https://flow.org/en/docs/types/classes/

Looks like it was initially right. Probably needed :boolean type for wasAnimating or something like that...

@ifndefdeadmau5
Copy link
Contributor Author

Yeah, that links makes lot of sense! Thank you so much. Learned bit of flow types today.

@nkbt
Copy link
Collaborator

nkbt commented Apr 25, 2017

Me too. I am going to use flow for react components too. So far was using it for only library code

@ifndefdeadmau5
Copy link
Contributor Author

Me either.Did not notice the benefit of Flow, just used proptypes in company project so far

nkbt
nkbt approved these changes Apr 25, 2017
View reviewed changes
Copy link
Collaborator

@nkbt nkbt left a comment

Choose a reason for hiding this comment

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

💯

@ifndefdeadmau5
Copy link
Contributor Author

Thank you for supporting this whole journey with me @nkbt. This PR was my first open source contribution

@nkbt
Copy link
Collaborator

nkbt commented Apr 25, 2017

🎉

@nkbt nkbt merged commit b0a7882 into chenglou:master Apr 25, 2017
@ifndefdeadmau5 ifndefdeadmau5 deleted the es6-class-migration branch April 25, 2017 12:15
@MichaelDeBoey
Copy link
Contributor

MichaelDeBoey commented Apr 25, 2017 •
edited
Loading

Test are still broken :/
Don't understand why they don't fail then :/
Will be fixed in #462 :-)

@snyk-bot snyk-bot mentioned this pull request Mar 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkbt nkbt nkbt approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@ifndefdeadmau5 @nkbt @MichaelDeBoey