Add support for an initial onboarding walkthrough

Add support for an initial onboarding walkthrough

- Submitting for Ant
- Under the feature flag :flags:/onboard

https://code.launchpad.net/~rharding/juju-gui/qa-ant-onboarding/+merge/191208

(do not edit description out of merge proposal)

[rharding, 2013-10-15 14:33:46]

Please take a look.

[rharding, 2013-10-15 14:55:34]

Feedback below.

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

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1095
app/app.js:1095: 
remove this blank line

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1129
app/app.js:1129: initialise_onboarding: function() {
initialize (I know it's a US/UK english thing, but initialize is used throughout
the app already)

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1131
app/app.js:1131: if (!this._onboarding) {
this is where I'd add the feature flag check.

if (!this._onboarding && window.flags.onboard) { ...

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1132
app/app.js:1132: // Need to check onboarding exists due to the double dispatch
bug.
this comment kind of goes above this line where the if is.

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1140
app/app.js:1140: this._onboarding.close();
why do we need to close here? If you hit the root url, and we render. You close
it with the button/control in the UI. If you're not at the / url then we never
show this and there's nothing to close?

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

https://codereview.appspot.com/14700043/diff/1/app/store/env/base.js#newcode170
app/store/env/base.js:170: 'onboardDismissed': {value: false},
you had mentioned that it's not saving this value. This comes in to where I'd
create a valueFn that could check against your storage location (session storage
or localstorage) and pull the value out of there. If it's not defined, then you
show, else you don't show. The render method would do a check when you hit it.

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

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode31
app/views/onboarding.js:31: var onboardingIndex = 0;
this is specific to the current view. I think this should be an attribute
created or set on initialize, or maybe another ATTR.

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode53
app/views/onboarding.js:53: '.onboarding-cross': {
can this not be done with css using :hover state?

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode71
app/views/onboarding.js:71: this.close();
is there any way to get back to the onboarding dialog after it's been closed?
Could this actually do a this.destroy() and remove itself from the dom as well?

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode94
app/views/onboarding.js:94: this.onboardingIndex = 0;
you set this.onboardingIndex here, but you use the unattached version elsewhere?

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcod...
app/views/onboarding.js:110: onboardingIndex = onboardingIndex + 1;
this.onboardingIndex

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcod...
app/views/onboarding.js:135: crossHandler: function(ev) {
yea, I'd think this whole function could go away if the css was updated per
https://developer.mozilla.org/en-US/docs/Web/CSS/:hover

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcod...
app/views/onboarding.js:166: container_bg.addClass('state-' + onboardingIndex);
this.onboardingIndex

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less#newc...
lib/views/stylesheet.less:526: &.displayNone {
there's already a .hidden class. Could that be used for this purpose vs adding a
new displayNone class?

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less#newc...
lib/views/stylesheet.less:1997: z-index: 2;
z-index 2? It's sitting inside an element of z-index 10001?

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less#newc...
lib/views/stylesheet.less:2043: &.state-1 {
this seems like it could be done using a .active flag or something. Where one of
the divs is marked .active and .active picks up things like display block and
sets background color?

[rharding, 2013-10-15 15:00:22]

QA Notes:

I didn't get the mask until I hit "Get Started" it seems like it would mask to
start out with?

Once when I got to the second step did it mask over the sidebar?

[Ant, 2013-10-15 16:13:05]

Comments back to Rick's review. Any uncommented are complete.

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

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1140
app/app.js:1140: this._onboarding.close();
On 2013/10/15 14:55:34, rharding wrote:
> why do we need to close here? If you hit the root url, and we render. You
close
> it with the button/control in the UI. If you're not at the / url then we never
> show this and there's nothing to close?

This is used when the user use's the browsers navigation buttons to jump form
build and browse.

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

https://codereview.appspot.com/14700043/diff/1/app/store/env/base.js#newcode170
app/store/env/base.js:170: 'onboardDismissed': {value: false},
On 2013/10/15 14:55:34, rharding wrote:
> you had mentioned that it's not saving this value. This comes in to where I'd
> create a valueFn that could check against your storage location (session
storage
> or localstorage) and pull the value out of there. If it's not defined, then
you
> show, else you don't show. The render method would do a check when you hit it.

I created this value in the storage for this purpose but did not manage to set
or get it from app.js when rendering.

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

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode53
app/views/onboarding.js:53: '.onboarding-cross': {
On 2013/10/15 14:55:34, rharding wrote:
> can this not be done with css using :hover state?

These events are used to update the spirited background image, therefore need to
update the class on the element.

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode71
app/views/onboarding.js:71: this.close();
On 2013/10/15 14:55:34, rharding wrote:
> is there any way to get back to the onboarding dialog after it's been closed?
> Could this actually do a this.destroy() and remove itself from the dom as
well?

There is going to be a drop down in the header that contains an option to
display the onboarding message again.

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcod...
app/views/onboarding.js:135: crossHandler: function(ev) {
On 2013/10/15 14:55:34, rharding wrote:
> yea, I'd think this whole function could go away if the css was updated per
> https://developer.mozilla.org/en-US/docs/Web/CSS/:hover

Required for sprite updates.

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/14700043/diff/1/lib/views/stylesheet.less#newc...
lib/views/stylesheet.less:2043: &.state-1 {
On 2013/10/15 14:55:34, rharding wrote:
> this seems like it could be done using a .active flag or something. Where one
of
> the divs is marked .active and .active picks up things like display block and
> sets background color?

The reason I did it this way is because the different states control the
screen-mask which I outside the panels.

[rharding, 2013-10-15 16:27:00]

Comment

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

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1140
app/app.js:1140: this._onboarding.close();
On 2013/10/15 16:13:05, Ant wrote:
> On 2013/10/15 14:55:34, rharding wrote:
> > why do we need to close here? If you hit the root url, and we render. You
> close
> > it with the button/control in the UI. If you're not at the / url then we
never
> > show this and there's nothing to close?
> 
> This is used when the user use's the browsers navigation buttons to jump form
> build and browse.

When they go from browse to build though the url changes and will no longer be
/. So I think it's safe to leave this out.

[Ant, 2013-10-15 16:48:56]

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

https://codereview.appspot.com/14700043/diff/1/app/app.js#newcode1140
app/app.js:1140: this._onboarding.close();
On 2013/10/15 16:27:00, rharding wrote:
> On 2013/10/15 16:13:05, Ant wrote:
> > On 2013/10/15 14:55:34, rharding wrote:
> > > why do we need to close here? If you hit the root url, and we render. You
> > close
> > > it with the button/control in the UI. If you're not at the / url then we
> never
> > > show this and there's nothing to close?
> > 
> > This is used when the user use's the browsers navigation buttons to jump
form
> > build and browse.
> 
> When they go from browse to build though the url changes and will no longer be
> /. So I think it's safe to leave this out.

Ok, taking it out.

[Ant, 2013-10-15 16:50:37]

On 2013/10/15 15:00:22, rharding wrote:
> QA Notes:
> 
> I didn't get the mask until I hit "Get Started" it seems like it would mask to
> start out with?
> 
> Once when I got to the second step did it mask over the sidebar?

That is a UX/design decision. It is the behaviour I was given to implement.
Testing may prove you right :)

[rharding, 2013-10-16 12:36:55]

Please take a look.

[rharding, 2013-10-16 16:53:59]

Please take a look.

[rharding, 2013-10-16 17:01:10]

LGTM, couple of small tweaks I think.

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#newcode84
app/app.js:84: onboarding: {
we manually run the view so this doesn't need to be here.

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#newc...
app/store/env/base.js:170: 'onboardDismissed': {value: false},
is this used? If not can we get rid of this as well?

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#ne...
app/views/onboarding.js:175: this.onboardingIndex + 1, this.stateCount);
this should just be this.states.length

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#ne...
app/views/onboarding.js:204: this.stateCount = this.states.length - 1;
I'd just move this up to the incrementIndex since you don't need/use it
elsewhere.

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#ne...
test/test_onboarding.js:159: onboard.closeHandler({halt: function() {}});
What you can do here is to do a onboardingCross.simulate('click') and it should
fire the click event and it'll close.

[gary.poster, 2013-10-16 17:52:32]

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#newc...
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#ne...
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#ne...
app/views/onboarding.js:119: * @method nextHandler
prevHandler

https://codereview.appspot.com/14700043/diff/15001/app/views/onboarding.js#ne...
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#ne...
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#ne...
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#ne...
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#ne...
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#ne...
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> :-)

[gary.poster, 2013-10-16 19:00:39]

QA not ok, but if you apply the following patch and write associated tests, it
will be good.

http://paste.ubuntu.com/6247161/

This makes the flag handling much less fragile
(http://localhost:8888/sidebar/:flags:/charmworldv3/onboard/ shows onboarding as
it should, for instance) and disables onboarding for the fullscreen view, where
it will be broken.  Please merge this and then write a few associated tests.

Thank you!

[rharding, 2013-10-17 15:05:33]

Please take a look.

[Ant, 2013-10-17 15:07:55]

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#ne...
app/views/onboarding.js:136: crossHandler: function(ev) {
On 2013/10/16 17:52:32, gary.poster wrote:
> (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.

1. The mouse up event calls closeHandler
2. I'll leave this for now but happy to update if required
3. Brilliant, done.
4. The close-inspector-click displays a brighter cross.

[rharding, 2013-10-17 15:20:57]

*** Submitted:

Add support for an initial onboarding walkthrough

- Submitting for Ant
- Under the feature flag :flags:/onboard

R=Ant, gary.poster
CC=
https://codereview.appspot.com/14700043