A dropdown view extension

A dropdown view extension

This is a refactoring of Huw's branch which created a dropdown widget
and a "Help & Feedback" dropdown on the header bar.

original: https://codereview.appspot.com/23410043/

https://code.launchpad.net/~hatch/juju-gui/help-dropdown/+merge/194915

(do not edit description out of merge proposal)

[jeff.pihach, 2013-11-12 18:48:17]

Please take a look.

[rharding, 2013-11-12 19:06:27]

Comments below, will QA. I've got some questions in there to look at please. Let
me know if a call will make things easier to explain.

I also think the new function (reset) should have a test in the onboarding
tests. I know you 'test' it in the drop down, but you mock out what it does and
just check it's called. The real test in onboarding should check localstorage is
cleared, the state is back at 0, etc.

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

https://codereview.appspot.com/25390043/diff/1/app/app.js#newcode620
app/app.js:620: _renderHelpDropdownView: function() {
does this need to be destroyed?

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
File app/assets/javascripts/view-dropdown-extension.js (right):

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:62: __bindUI:
function(container) {
is this two underscores? I think the convention of one is ok. Is this just to
prevent issues in applying the extension? Maybe prefix instead?

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:76: _addDropdownFunc:
function() {
should this be private then? hmmm, I guess it's private to the View using the
extension.

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:89: 'event-tracker',
does this work right? The event-tracker is meant to be part of the extension
chain of the View. You've included it here, but it's not actually using it.

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js
File app/views/help-dropdown.js (right):

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js#new...
app/views/help-dropdown.js:64: onboarding.render();
 how does this handle in fullscreen view?

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js#new...
app/views/help-dropdown.js:104: 'juju-view-utils',
this should have the event tracker here. It's directly including it.

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

https://codereview.appspot.com/25390043/diff/1/app/views/notifications.js#new...
app/views/notifications.js:261: 'view-dropdown-extension'
again, it needs the event tracker.

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.js
File test/test_dropdown_extension.js (right):

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.j...
test/test_dropdown_extension.js:32: View = Y.Base.create('dropdown', Y.View,
[Y.juju.Dropdown], {
so then this needs the EventTracker in it.

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.j...
test/test_dropdown_extension.js:33: template: Y.Node.create([
how does this work? the template is called this.template() and a Node isn't
callable?

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.j...
test/test_dropdown_extension.js:63:
container.one('.menu-link').simulate('click');
I don't see how this works? The template is never put into the DOM that I can
tell. _addDropdownFunc doesn't use the template in any way.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js
File test/test_help_dropdown.js (right):

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:23: var views, models, db, landscape, Y, viewNode,
envAnno, helpView;
sort please, makes it a lot easier to see what's defined and if it's already
been defined.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:45: viewNode = Y.Node.create('<div
id="help-dropdown"></div>');
why not makeContainer with the id passed in as the argument?

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:51: helpView.destroy();
this should be an if helpView.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:81: it('can start the onboarding visualization',
function(done) {
like before, this should be dependent on the viewmode being sidebar I think.

[rharding, 2013-11-12 19:18:58]

Thanks for the updates and refactoring. I think it's on the right track but I do
have a few questions and such in the notes. QA looks ok other than the
tutorial/fullscreen issue noted.

[jeff.pihach, 2013-11-12 20:28:23]

Thanks for the review and QA. The fixes are incoming and I added a test for the
onboarding reset.

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

https://codereview.appspot.com/25390043/diff/1/app/app.js#newcode620
app/app.js:620: _renderHelpDropdownView: function() {
It should live as long as the app does but it couldn't hurt for tests. done

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
File app/assets/javascripts/view-dropdown-extension.js (right):

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:62: __bindUI:
function(container) {
It is, I was trying it out, see how it felt :)

The issue with extensions is that you may accidentally overwrite a method in the
parent. This prevents it because nobody ever uses double underscore. I had tried
prefixing but it just looked ugly..I'd be open to suggestions.

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:76: _addDropdownFunc:
function() {
Right, because this gets 'mixed in' it's private to the view using the
extension. (at least that's my thought process)

https://codereview.appspot.com/25390043/diff/1/app/assets/javascripts/view-dr...
app/assets/javascripts/view-dropdown-extension.js:89: 'event-tracker',
It's just there to illustrate to the user that this requires that module, it
doesn't have any effect on the execution.

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js
File app/views/help-dropdown.js (right):

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js#new...
app/views/help-dropdown.js:64: onboarding.render();
It doesn't - good catch! :)

https://codereview.appspot.com/25390043/diff/1/app/views/help-dropdown.js#new...
app/views/help-dropdown.js:104: 'juju-view-utils',
right thanks, done

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

https://codereview.appspot.com/25390043/diff/1/app/views/notifications.js#new...
app/views/notifications.js:261: 'view-dropdown-extension'
done

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.js
File test/test_dropdown_extension.js (right):

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.j...
test/test_dropdown_extension.js:32: View = Y.Base.create('dropdown', Y.View,
[Y.juju.Dropdown], {
On 2013/11/12 19:06:27, rharding wrote:
> so then this needs the EventTracker in it.

Done.

https://codereview.appspot.com/25390043/diff/1/test/test_dropdown_extension.j...
test/test_dropdown_extension.js:33: template: Y.Node.create([
On 2013/11/12 19:06:27, rharding wrote:
> how does this work? the template is called this.template() and a Node isn't
> callable?

Done.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js
File test/test_help_dropdown.js (right):

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:23: var views, models, db, landscape, Y, viewNode,
envAnno, helpView;
On 2013/11/12 19:06:27, rharding wrote:
> sort please, makes it a lot easier to see what's defined and if it's already
> been defined. 

Done.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:45: viewNode = Y.Node.create('<div
id="help-dropdown"></div>');
Artifact of the refactor, fixed.

https://codereview.appspot.com/25390043/diff/1/test/test_help_dropdown.js#new...
test/test_help_dropdown.js:51: helpView.destroy();
On 2013/11/12 19:06:27, rharding wrote:
> this should be an if helpView. 

Done.

[jeff.pihach, 2013-11-12 21:34:14]

Please take a look.

[rharding, 2013-11-12 22:01:14]

This looks a lot better. Some small notes below. Will QA.

https://codereview.appspot.com/25390043/diff/20001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/app.js#newcode623
app/app.js:623: env: this.db.environment
why is the env passed in?

https://codereview.appspot.com/25390043/diff/20001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:637: if (force) {
this can be simpler. Just 

if (force) { .reset() }

if (!this._onboarding.get('seen')) { .render() }

since the reset will make the seen false.

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js
File app/views/help-dropdown.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js...
app/views/help-dropdown.js:64: // once fullscreen is removed we can interact
with
caps

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js...
app/views/help-dropdown.js:65: // onboarding here instead of in browser.js
ugh, disagree, we still don't want this view directly effecting another.

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js...
app/views/help-dropdown.js:97: return this;
why the return?

https://codereview.appspot.com/25390043/diff/20001/app/views/notifications.js
File app/views/notifications.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/views/notifications.js...
app/views/notifications.js:211: @property dropdown
there is no property dropdown that I see.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js
File test/test_help_dropdown.js (right):

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:47: Y.one('body').append(viewNode);
makeContainer already adds the node to the body.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:59: container: Y.one('#help-dropdown'),
this can just be viewNode then?

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:73: container: Y.one('#help-dropdown'),
and here

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:86: container: Y.one('#help-dropdown'),
and here

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:90: helpView.fire = function(type, cfg) {
can't this just be helpView.on('navigate', function() { all the tests} and then
not need the oldfire vs newfire business?

[rharding, 2013-11-12 22:04:16]

LGTM QA-ok with the notes from the previous comment.

[jeff.pihach, 2013-11-12 22:15:01]

Thanks for the review and qa, the fixes are coming soon.

https://codereview.appspot.com/25390043/diff/20001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/app.js#newcode623
app/app.js:623: env: this.db.environment
To calculate the landscape url

https://codereview.appspot.com/25390043/diff/20001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:637: if (force) {
On 2013/11/12 22:01:15, rharding wrote:
> this can be simpler. Just 
> 
> if (force) { .reset() }
> 
> if (!this._onboarding.get('seen')) { .render() }
> 
> since the reset will make the seen false. 

Done.

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js
File app/views/help-dropdown.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js...
app/views/help-dropdown.js:64: // once fullscreen is removed we can interact
with
On 2013/11/12 22:01:15, rharding wrote:
> caps

Done.

https://codereview.appspot.com/25390043/diff/20001/app/views/help-dropdown.js...
app/views/help-dropdown.js:97: return this;
chaining, see the tests.

https://codereview.appspot.com/25390043/diff/20001/app/views/notifications.js
File app/views/notifications.js (right):

https://codereview.appspot.com/25390043/diff/20001/app/views/notifications.js...
app/views/notifications.js:211: @property dropdown
right, I forgot to remove. Nice catch.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js
File test/test_help_dropdown.js (right):

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:47: Y.one('body').append(viewNode);
On 2013/11/12 22:01:15, rharding wrote:
> makeContainer already adds the node to the body. 

Done.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:59: container: Y.one('#help-dropdown'),
On 2013/11/12 22:01:15, rharding wrote:
> this can just be viewNode then? 

Done.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:73: container: Y.one('#help-dropdown'),
On 2013/11/12 22:01:15, rharding wrote:
> and here

Done.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:86: container: Y.one('#help-dropdown'),
On 2013/11/12 22:01:15, rharding wrote:
> and here

Done.

https://codereview.appspot.com/25390043/diff/20001/test/test_help_dropdown.js...
test/test_help_dropdown.js:90: helpView.fire = function(type, cfg) {
On 2013/11/12 22:01:15, rharding wrote:
> can't this just be helpView.on('navigate', function() { all the tests} and
then
> not need the oldfire vs newfire business?

Done.

[jeff.pihach, 2013-11-12 22:21:21]

Please take a look.

[jeff.pihach, 2013-11-12 23:26:25]

Please take a look.

[jeff.pihach, 2013-11-12 23:44:03]

Please take a look.

[jeff.pihach, 2013-11-13 16:49:07]

Please take a look.

[matthew.scott, 2013-11-13 17:06:19]

LGTM, QA okay!

https://codereview.appspot.com/25390043/diff/90001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/25390043/diff/90001/app/subapps/browser/browse...
app/subapps/browser/browser.js:629: * @param {Boolean} force Weather it should
force render the onboarding.
s/Weather/Whether

[jeff.pihach, 2013-11-13 17:08:52]

Thanks everyone for the reviews and qa's!