search

Famous / engine

MIT License
1.75k stars 250 forks source link

Don't check an instance constructor for static properties since that might not be the same when classes extend. #386

Closed trusktr closed 9 years ago

trusktr commented 9 years ago

Fixes #385

alexanderGugel commented 9 years ago

LGTM. Copying over the static properties from the Node to all subclasses or decorating the constructor in the Node constructor would also be an alternative. Probably won't be prettier.

jd-carroll commented 9 years ago

Please do not merge this, it will break proportional sizing.

For reference please look at: #354 Also #353 & #350

trusktr commented 9 years ago

@jd-carroll Good catches! I wonder what's the best approach to this. @alexanderGugel I like your idea to decorate the constructor (if the property isn't already defined on the constructor).

trusktr commented 9 years ago

@jd-carroll @alexanderGugel Made changes. What do you think?

jd-carroll commented 9 years ago

This is exactly what I had in my original PR #355. However, in discussions with @DnMllr he identified that the constants are unnecessary and should actually be removed in favor of the constants in Size.

I also just realized, this PR is against the wrong branch. Should be merged with develop not master. If this was merged against develop then you would see the conflict because this case has already been addressed.

@trusktr Please update your dependency in your project to:

  ...
  "dependencies": {
    "babelify": "^6.0.1",
    "famous": "git://github.com/famous/engine#develop"
  }

If you still have problems after updating your dependency please repost.

trusktr commented 9 years ago

@jd-carroll Oops! Closing this since it's on master.

trusktr commented 9 years ago

@alexanderGugel @jd-carroll @DnMllr Continuing at #387.