Code review comment for lp:~rharding/juju-gui/qa-ant-onboarding

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#newcode110
app/views/onboarding.js:110: onboardingIndex = onboardingIndex + 1;
this.onboardingIndex

https://codereview.appspot.com/14700043/diff/1/app/views/onboarding.js#newcode135
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#newcode166
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#newcode526
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#newcode1997
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#newcode2043
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?

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