...
[15:39:42 CST(-0600)] <Bosmon> So, rather than have a block of logic which indexes various strategies by testing against string constants, each alternative should be encapsulated as an IoC configured subcomponent of the overall component
[15:40:29 CST(-0600)] <colinclark> Bosmon, michelled, anastasiac: These are excellent review points
[15:40:47 CST(-0600)] <colinclark> very helpful
[15:40:54 CST(-0600)] <colinclark> and they make a ton of sense to me
[15:41:16 CST(-0600)] <michelled> yes, this is a good and clear strategy
[15:41:28 CST(-0600)] <Bosmon> As a wider review point, I would recommend factoring out the entirety of "bindKeyboardControl" into a block of configuration
[15:41:35 CST(-0600)] <Bosmon> Rather than a method which produces configuration
[15:42:15 CST(-0600)] <Bosmon> Similarly, the "defaultKeys" structure on line 99 should become a globally visible configuration block
[15:42:50 CST(-0600)] <Bosmon> It looks like this whole "controls" concept is a bit "ant-like" (in terms of our ideas about UIOptions) given it is a switch which is used in multiple places
[15:42:57 CST(-0600)] <Bosmon> For example, it appears again in "produceTree"
[15:43:19 CST(-0600)] <Bosmon> A suitable strategy for dealing with that role of it would be to have the base component fire an event, called something like "onProduceTree"
[15:43:36 CST(-0600)] <Bosmon> The new subcomponents we make can have their effect at this time by subscribing to the event and contributing their material to the tree
[15:44:26 CST(-0600)] <Bosmon> Oh wait, I misread.... the ONLY place this is used is inside produceTree
[15:44:28 CST(-0600)] <Bosmon> So, that is fine
[15:45:19 CST(-0600)] <Bosmon> The "diff" view made me think that logic like this is actually part of bindKeyboardControl - but that's actually a completely separate review point
[15:47:15 CST(-0600)] <Bosmon> On line 264, "that.fullscreen" is a suspicious kind of top-level method to have on a component - since it's not actually suitable for user use
[15:48:07 CST(-0600)] <Bosmon> Seeing it there, the user might think that they could call it to change the fullscreen condition of the component, but actually they can't