LGTM if you change the route check code in app.js.  I'll try to look up
the proper spelling and post it here in a few minutes.  Everything else
is trivial and/or just suggestions or ideas.


https://codereview.appspot.com/14700043/diff/15001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/14700043/diff/15001/app/app.js#newcode1132
app/app.js:1132: if (path === '/' || path === '/:flags:/onboard/') {
Technically, this should use the routing code to get the path in the
default namespace.  I'll try to dig up how to do that later.

I say "technically" because we are interested in removing the namespace
code.  We aren't doing that right now, though, so using the routing's
parsing code is the right thing to do.

https://codereview.appspot.com/14700043/diff/15001/app/store/env/base.js
File app/store/env/base.js (right):

https://codereview.appspot.com/14700043/diff/15001/app/store/env/base.js#newcode170
app/store/env/base.js:170: 'onboardDismissed': {value: false},
On 2013/10/16 17:01:11, rharding wrote:
> is this used? If not can we get rid of this as well?

Looks like it is part of a nascent plan to persist the flag, but I agree
that we ought to keep this out until/unless it's used.

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js
File app/views/onboarding.js (right):

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#newcode72
app/views/onboarding.js:72: this.close();
I don't yet see the value of separating closeHandler and close.  Even
for testing this doesn't seem like a huge win.  OTOH, maybe I'm missing
something.

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#newcode119
app/views/onboarding.js:119: * @method nextHandler
prevHandler

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#newcode136
app/views/onboarding.js:136: crossHandler: function(ev) {
(1) this is also registered for mouseup.  Why?
(2) Other than the blasted documentation blather :-D, this feels like it
would be cleaner as separate methods, one for each event.  That's a
suggestion: take it or leave it.
(3) .replaceClass(..., ...) could make each type into single calls.
(4) I hope the close-inspector-click style wins over the -hover or
-normal styles, or that they get along somehow.  I'll assume they do.

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#newcode175
app/views/onboarding.js:175: this.onboardingIndex + 1, this.stateCount);
On 2013/10/16 17:01:11, rharding wrote:
> this should just be this.states.length

you mean, "this.stateCount" should be "this.states.length - 1".  Sounds
good to me.

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#newcode204
app/views/onboarding.js:204: this.stateCount = this.states.length - 1;
On 2013/10/16 17:01:11, rharding wrote:
> I'd just move this up to the incrementIndex since you don't need/use
it
> elsewhere.

+1

https://codereview.appspot.com/14700043/diff/15001/test/test_onboarding.js
File test/test_onboarding.js (right):

https://codereview.appspot.com/14700043/diff/15001/test/test_onboarding.js#newcode74
test/test_onboarding.js:74: assert.equal(onboard.onboardingIndex, 2);
You could show that trying to go past the length doesn't mess anything
up, since you have code for that, yeah?  As you wish.

https://codereview.appspot.com/14700043/diff/15001/test/test_onboarding.js#newcode87
test/test_onboarding.js:87: assert.equal(onboard.onboardingIndex, 0);
You could show that trying to go lower than 0 doesn't mess anything up,
too.  As you wish.

https://codereview.appspot.com/14700043/diff/15001/test/test_onboarding.js#newcode111
test/test_onboarding.js:111:
assert.isTrue(background.hasClass('state-1'), 'should be state 1');
you could write a helper that verifies that the class *only* has the
desired states, and not the others.  <shrug> :-)

https://codereview.appspot.com/14700043/