Moves viewmode control binding to a mixin

Moves viewmode control binding to a mixin

* A new mixin has been created
* Nav functions like _goFullscreen have been moved to the mixin
* * toggleMinimized is just a placeholder, as fullscreen/sidebar need to do a
different thing than minimized
* The mixin doesn't create controls; it just has them passed in, since different
views setup the controls differently.

https://code.launchpad.net/~jcsackett/juju-gui/view-controls-mixin/+merge/181615

(do not edit description out of merge proposal)

[j.c.sackett, 2013-08-22 16:45:29]

Please take a look.

[rharding, 2013-08-22 16:55:06]

Thanks for the cleanup.

I'd suggest keeping the extension in the widget code base. It's tied to the
widget and needs to work with it. The Views are already importing that module in
order to display the widget. The extension comes along for free. Then the widget
tests can consume the tests for the extension as well to verify things work as
they should work tied together. It's a pattern setup by the overlay-indicator
widget.

Naming-wise I'd suggest something like ViewModeControlsView(Extension|Helper) or
something.


The tests used in the current views should probably be pulled into testing this
exntension directly. You could create an empty Y.View class, stick the extension
on it, and make sure that the events fire with the right payload as it done
currently in testing fullscreen/sidebar. Then they can not be duped in
individual fullscreen/minimized/sidebar views.

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

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1484
app/views/utils.js:1484: _bindViewmodeControls: function(controls) {
you pass in controls, but continue to use this.controls for the events.

[matthew.scott, 2013-08-22 18:52:39]

LGTM with Rick's suggestions - good code and ideas; I'm good with Rick's
changes, too.

[jeff.pihach, 2013-08-22 21:03:31]

LGTM with rick's comments about testing

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view...
app/subapps/browser/views/view.js:44: Y.juju.views.utils.viewmodeControllingView
this can be shortened to just 'views' as it's defined above

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view...
app/subapps/browser/views/view.js:227: _toggleMinimized: function(ev) {
I note that this is called by the extension would be helpful.

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

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1471
app/views/utils.js:1471: * Constructor
This is technically incorrect, the constructor is the function on 1466

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1475
app/views/utils.js:1475: _initViewmodeControllingView: function() {
noop?

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1536
app/views/utils.js:1536: * @method _toggle_sidebar
_toggleMinimized

[j.c.sackett, 2013-08-22 21:34:59]

Please take a look.

[rharding, 2013-08-23 13:08:13]

LGTM with comment on naming and a check that there's test cases for the
minimized method required by the two views fullscreen/sidebar.

https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/m...
File app/subapps/browser/views/minimized.js (right):

https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/m...
app/subapps/browser/views/minimized.js:40:
Y.juju.widgets.ViewmodeControllingViewExtension
The widget is ViewmodeControls, I'd stick to that vs Controlling.

https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls...
File test/test_viewmode_controls_widget.js (right):

https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls...
test/test_viewmode_controls_widget.js:173: view._toggleMinimized = function() {
so we need individual tests on this then since it's not implemented in the
extension right?

[j.c.sackett, 2013-08-23 13:36:11]

On 2013/08/23 13:08:13, rharding wrote: 
>
https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/m...
> app/subapps/browser/views/minimized.js:40:
> Y.juju.widgets.ViewmodeControllingViewExtension
> The widget is ViewmodeControls, I'd stick to that vs Controlling.

Made that switch.

>
https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls...
> test/test_viewmode_controls_widget.js:173: view._toggleMinimized = function()
{
> so we need individual tests on this then since it's not implemented in the
> extension right?

You're right; I hadn't intended to *completely* delete the minimzed tests. Put
one back in for the toggle to sidebar. Sidebar already has a test for toggling
to minimized.

[j.c.sackett, 2013-08-23 13:43:08]

*** Submitted:

Moves viewmode control binding to a mixin

* A new mixin has been created
* Nav functions like _goFullscreen have been moved to the mixin
* * toggleMinimized is just a placeholder, as fullscreen/sidebar need to do a
different thing than minimized
* The mixin doesn't create controls; it just has them passed in, since different
views setup the controls differently.

R=rharding, matthew.scott, jeff.pihach
CC=
https://codereview.appspot.com/12935046