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

Revision history for this message
Anthony Dillon (ya-bo-ng) wrote :

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#newcode135
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#newcode2043
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.

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

« Back to merge proposal