Skip to content

Conversation

@trusktr
Copy link
Contributor

@trusktr trusktr commented Jul 3, 2015

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

@trusktr I still don't think that this change is necessary. The original problem was that subclasses had to opt-in to receive the default components. Since this has been changed, I don't feel that this change is necessary (and would rather see those sizing constants removed).

The heart of this issue absolutely needs to be kept in mind moving forward (subclass inheritance). An examination of all "subclassable" elements should be performed to ensure there aren't any other problems out there.

@trusktr
Copy link
Contributor Author

trusktr commented Jul 3, 2015

@jd-carroll I see what you mean. It doesn't really make sense to change

Node.RELATIVE_SIZE = 0;
Node.ABSOLUTE_SIZE = 1;
Node.RENDER_SIZE = 2;

in a subclass, since those are like an enum, but

Node.DEFAULT_SIZE = 0;

seems like a configurable value. Does it make sense to have the Size constants, when those are more difficult to type than just 'absolute', 'relative', etc?

I think that these conditional checks are a general pattern to make sure they'll be accessed properly using this.constructor in base classes. Can we expect someone to make a new class NewNode that extends Node, on that constructor do NewNode.DEFAULT_SIZE = 1;, and reference this.constructor.DEFAULT_SIZE in a statement that will fail if a second person extends NewNode with AnotherNewNode and doesn't set AnotherNewNode.DEFAULT_SIZE to anything?

@trusktr
Copy link
Contributor Author

trusktr commented Jul 3, 2015

I'm gonna close this since at least my original problem in #385 is gone now that Node uses the double negative check (! + NO_...=false) to catch undefined values.

@trusktr trusktr closed this Jul 3, 2015
@trusktr trusktr deleted the issue-385 branch August 19, 2015 23:38
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.

2 participants