initial commit

[kintel]

No description provided.

[sunjay]

Should update this section to talk specifically about shared views

[sunjay]

I've been avoiding loading big libraries like lodash into our code for fear of bloating our built code. For the most part, I've found it unnecessary. See my comments below on the _. lines.

[kintel]

I expected lodash entry points to be picked out by browserify, including only the actually used code paths in to our build code. Is that not the case?

[sunjay]

I don't think browserify does that? The more important concern though is that it really isn't necessary in this case. The _.range usage isn't very extensible and we already provide properties for that.

[sunjay]

This should use the LightArray#stripIds property and the panelIds property on each strip instead of an array and _.range(). See panel-view.js for an example of usage.

[sunjay]

Since the code to get these file names is repeated, can you put it in some method and move the directory path to config?

[sunjay]

Note: when moving this into the config you're not going to have access to the store. You should adopt a similar approach to what I've done in default-config.js in game-logic. Take a look at the CONTROL_MAPPINGS definition.

[kintel]

Why should they be LightArray#stripIds properties? These are not tracked data, merely a configuration which are only used internally in the audio view. Not sure why arrays should be avoided in this case.

[sunjay]

Think of those properties like metadata on the tracked data. That array gives you access to all of the stripIds created. It doesn't need to be tracked because it doesn't change. It's distinct from a constant though because it is local to each light array and doesn't apply to all light arrays.

[kintel]

I'm not sure what you mean. Could you write a code snippet explaining how you would populate such a data structure?

[sunjay]

Sure. Since this should probably eventually end up in config anyway, I would populate it like this:

// in config.js constructor:
this.SOUNDS = {
  //...
  MOLE: {
    //...
    PANEL_ACTIVE: {
      // stripId
      '0': {
          // Note: SOUNDS_PATH.MOLE_GAME = "sounds/Game_01"
          // panelId
          '0': join(this.SOUNDS_PATH.MOLE_GAME, 'G01_LED_001.wav'),
          '1': join(this.SOUNDS_PATH.MOLE_GAME, 'G01_LED_002.wav'),
          // Example revised sound
          '2': join(this.SOUNDS_PATH.MOLE_GAME, 'G01_LED_003_rev.wav'),
          '3': join(this.SOUNDS_PATH.MOLE_GAME, 'G01_LED_004.wav'),
      }
    }
  }
  //...
}
// Note that this is just an example so you may come up with better names than I just did. :) 

There's a number of reasons I'd write the code like this and it has several benefits.

All of these numbers that represent the strips and panels, though they look sequential, don't have to be. They're IDs with no defined order. We may have to change panel ID 2 to 7 one day because something went wrong. We shouldn't work on the assumption that they are sequential in our code.

Another reason I'd do it like this is because while this may seem to be unnecessarily repetitive, it gives us a lot of freedom to configure the sounds individually however we like. Someone does the work to write this out once and we can adjust every sound whenever we want. In the code I have above, we're not limited by the seemingly sequential nature of the IDs that I was describing before. We can also have whatever filenames we want.

Imagine a scenario where we had two versions of a single panel's sound. What if we need to test that revision and want to swap a "_rev" prefix into the filename somewhere. It's worth it to have all of this laid out like this in the configuration class.

You can abstract out the name building as much as you'd like, even more than I did if it makes it easier. Just make sure that it's easy to configure and change.

This way of doing it also makes it easy to reason about and access from the code. You can simply use [stripId][panelId] on the relevant part of your configuration object. See how CONTROL_MAPPINGS is used in the disk view code.

[sunjay]

Same with all of these paths. Could you move these to config? Also, if you need to do this, be careful when concatenating paths because we could run into weird bugs with / being forgotten and whatnot.

[sunjay]

Rather than repeating these loops here, could you put the loops in the outermost context and then put the if statements inside? That way you only have to traverse the lightArray once.

[kintel]

Consider the event handlers temporary code for now. We need changes to game logic to be able to do the right thing. There's a separate card on trello for that.

[sunjay]

By event handlers do you mean change handlers? (Sorry I'm not sure what code you're referring to.)
If you're referring to what I think you are referring to:
Even if it is temporary code, moving the conditions inside the loops will dramatically decrease the amount of code you have in that handler and make everyone's lives easier for when this temporary code becomes "real" code.

[kintel]

I'll remove the temporary code for now, leaving us with infrastructure.

[sunjay]

This isn't defined in the constructor. Please make sure you do that.