Skip to content

Conversation

@trusktr
Copy link
Contributor

@trusktr trusktr commented Jul 3, 2015

Fixes #385

@trusktr trusktr changed the title Don't check an instance constructor since that might not be the same when classes extend. Don't check an instance constructor for static properties since that might not be the same when classes extend. Jul 3, 2015
@alexanderGugel
Copy link
Member

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
Copy link
Contributor

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

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

@trusktr
Copy link
Contributor Author

trusktr commented Jul 3, 2015

@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 trusktr force-pushed the issue-385 branch 2 times, most recently from 4e3773e to 664e73d Compare July 3, 2015 18:15
@trusktr
Copy link
Contributor Author

trusktr commented Jul 3, 2015

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

…wn settings with Node being the source of default settings.
@jd-carroll
Copy link
Contributor

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
Copy link
Contributor Author

trusktr commented Jul 3, 2015

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

@trusktr trusktr closed this Jul 3, 2015
@trusktr
Copy link
Contributor Author

trusktr commented Jul 3, 2015

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

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.

3 participants