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

src/animation: Convert to es6 classes #20014

Merged
merged 7 commits into from
Feb 19, 2021

Conversation

ianpurvis
Copy link
Contributor

Supports #19986

@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Aug 5, 2020
43 tasks
@ianpurvis ianpurvis force-pushed the animation-es6-classes branch from e707daf to 3afe5c5 Compare August 8, 2020 15:33
@ianpurvis ianpurvis changed the title Convert to animation/ es6 classes src/animation: Convert to es6 classes Aug 8, 2020
@ianpurvis ianpurvis force-pushed the animation-es6-classes branch 2 times, most recently from e6f0d31 to 1d9b33c Compare August 13, 2020 21:25
@ianpurvis ianpurvis force-pushed the animation-es6-classes branch from 1d9b33c to 73d6e25 Compare August 20, 2020 02:27
@mrdoob mrdoob added this to the r121 milestone Aug 23, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2021

@ianpurvis Do you mind resolving the merge conflicts?

@ianpurvis
Copy link
Contributor Author

@Mugen87 Will do!

@ianpurvis ianpurvis force-pushed the animation-es6-classes branch from 73d6e25 to 070b340 Compare February 18, 2021 14:48
@ianpurvis
Copy link
Contributor Author

Fyi, I discarded c4872f5 in light of #21293

@@ -118,14 +42,14 @@ Object.assign( AnimationClip, {

}

const clip = new AnimationClip( json.name, json.duration, tracks, json.blendMode );
const clip = new this( json.name, json.duration, tracks, json.blendMode );
Copy link
Owner

Choose a reason for hiding this comment

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

Hah! I didn't know you could do new this() 🤓

Copy link
Owner

Choose a reason for hiding this comment

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

Wait, I see you do new this.constructor a bit further down the file. What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! If you are constructing from a static method you can use this(), otherwise you have to use this.constructor()

class Foo {

  constructor(attributes) {
    Object.assign(this, attributes);
  }

  static create(attributes) {
    return new this(attributes);
  }

  clone() {
    return new this.constructor(this) ;
  }
}

class Bar extends Foo { }


const objects = [
 new Foo({ name: 'foo1' }),
 Foo.create({ name: 'foo2' }),
 new Foo({ name: 'foo3' }).clone(),
 new Bar({ name: 'bar1' }),
 Bar.create({ name: 'bar2' }),
 new Bar({ name: 'bar3' }).clone(),
]

for (const obj of objects) {
  console.log(obj);
}

Outputs -->

Foo { name: 'foo1' }
Foo { name: 'foo2' }
Foo { name: 'foo3' }
Bar { name: 'bar1' }
Bar { name: 'bar2' }
Bar { name: 'bar3' }

Copy link
Owner

Choose a reason for hiding this comment

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

Dang. You know your stuff! 🙏

@mrdoob
Copy link
Owner

mrdoob commented Feb 18, 2021

Apart from the new this and new this.constructor question, the PR seems good to go 👌

@ianpurvis
Copy link
Contributor Author

Caught and fixed an issue in 9c437fd 👍

@mrdoob mrdoob merged commit f4ea124 into mrdoob:dev Feb 19, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 19, 2021

Thanks!

@ianpurvis ianpurvis deleted the animation-es6-classes branch February 19, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants