Menu cleanup #4662

ellisonbg · 2013-12-07T23:46:47Z

We had a lot of stuff in the menus that was not really needed and some other stuff that was poorly organized. Here is a summary:

Removed the Kernel menu entirely. All kernel actions are in the toolbar.
Added a "Restart Kernel" button to the toolbar.
Removed the cell selection menu items from the "Cell" menu. The are again the toolbar.
Cleaned up the output management menu items in the "Cell" menu.
Removed the unneeded "Select Prev|Next" from the "Edit" menu.

Yeah simplification!

takluyver · 2013-12-07T23:56:11Z

I'm all in favour of keeping things properly organised, but the normal menu+toolbar paradigm is that toolbars are shortcuts to options that are also accessible through the menus, not an alternative to menus. I'm not convinced that we should depart from this without a good reason.

ellisonbg · 2013-12-08T00:44:39Z

That was definitely my original vision: everything was available in the
menus, toolbar was for most common things. But over time I have realized
that some of the menu items are never used and others are too much of a
violation of DRY. And then there is also the need to keep things as simple
as possible. I can definitely back pedal if needed of some of the changes,
but I would love to get a read from everyone first.

On Sat, Dec 7, 2013 at 3:56 PM, Thomas Kluyver notifications@github.comwrote:

I'm all in favour of keeping things properly organised, but the normal
menu+toolbar paradigm is that toolbars are shortcuts to options that are
also accessible through the menus, not an alternative to menus. I'm not
convinced that we should depart from this without a good reason.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4662#issuecomment-30070976
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

minrk · 2013-12-08T03:01:44Z

I'm with Thomas - I don't think there should be actions that are not in the menus. I think toolbars, keyboard shortcuts, etc. should all be duplicate entry points for convenience.

ellisonbg · 2013-12-08T03:19:02Z

Thanks for the feedback. Here is my modified thoughts:

Removing the "Kernel" menu entirely will likely confuse users. I am +0.8 on putting it back.
I am still +0 on putting the cell type items in the "Cell" menu, but don't feel strongly about it. I would, however, prefer to put it into a submenu. Thoughts?
I am -1 on including select next/prev in the "Edit" menu. These are equivalent to the up/down arrow keys and I have never seen another app put simple navigation like this in a menu. Also cell selection is completely equivalent to simply clicking on a cell. Requiring two clicks (click on "Edit" menu and then on "Select Next") instead of just one (just click on the cell!) is complete silliness.
Another thing that I don't think we should put into a menu - UI for switching between command/edit mode. I think this falls under the basic navigation category. This is not to mention that it won't work because clicking in the menu always puts the notebook into command mode.

ellisonbg · 2013-12-08T03:22:35Z

LOL, I guess the "Cell Type" items were already in a submenu. I will put that and the "Kernel" menu back...

minrk · 2013-12-08T03:22:50Z

I would, however, prefer to put it into a submenu. Thoughts?

My only thought on this is that bootstrap 3 removes submenus, if I recall correctly.

I am -1 on including select next/prev in the "Edit" menu.

Another thing that I don't think we should put into a menu - UI for switching between command/edit mode

I agree on navigation - these are not normal menu actions. I actually think it's a bit weird for them to be in the toolbar as well.

ellisonbg · 2013-12-08T03:25:31Z

Ouch on the Bootstrap 3 submenus...

I am a bit confused. We don't have "Select Next/Prev" in the toolbar. Do you mean "Move Up/Down"? I actually use those sometimes...

minrk · 2013-12-08T03:28:19Z

I am a bit confused. We don't have "Select Next/Prev" in the toolbar. Do you mean "Move Up/Down"? I actually use those sometimes...

I forgot what those arrows were for. Yes, move makes perfect sense, ignore me.

ellisonbg · 2013-12-08T06:52:36Z

OK I have added back the Kernel menu and Cell Type submenu. Waiting for Travis, but everything else should be ready to go.

Carreau · 2013-12-08T10:02:23Z

IPython/html/static/notebook/js/notebook.js

+    Notebook.prototype.expand_all_output = function () {
+        var ncells = this.ncells();
+        var cells = this.get_cells();
+        for (var i=0; i<ncells; i++) {


non need to get ncells, for (i in cells)will loop through the index. also, would it be possible to either use smth map or debounce, This is one of the call that might hang the notebook as the js is stuck in a tight for loop. (eventually call the expand_output in a set_timeout 0

Carreau · 2013-12-08T10:04:38Z

Looks good, agree to keep things in menu.

ellisonbg · 2013-12-08T21:11:49Z

OK loops are cleaned up.

damianavila · 2013-12-09T14:22:11Z

IPython/html/static/notebook/js/menubar.js

@@ -187,9 +181,11 @@ var IPython = (function (IPython) {
        // Insert
        this.element.find('#insert_cell_above').click(function () {
            IPython.notebook.insert_cell_above('code');
+            IPython.notebook.select_prev();


Why focus on the previous cell of the inserted cell?
It makes sense to insert and focus in the inserted, just IPython.notebook.insert_cell_above('code'); or
it would also makes sense to focus in the "origin" cell with IPython.notebook.select_prev(); so you are just inserting cell not inserting and focus... but the behaviour of the previous cell does not feel right to me 😉

IIRC this is because insert_cell_above doesn't actually focus that cell.

But insert_cell_above and insert_cell_below are both focusing in "destination" cell... both are calling insert_cell_at_index which is selecting itself this.select(this.find_cell_index(cell)); or am I misinterpreting something? (I do not get all the IPython js internals yet 😉)

I will double check. I think you may be right and I accidentally put these lines thinking this was my newux branch...

damianavila · 2013-12-09T15:19:53Z

Just a couple of comments...
Leaving aside my comments, it looked good when I tested.
Btw, I agree with keeping some things in the menu too.

fperez · 2013-12-10T08:11:57Z

+1 to keeping all functionality on the menus. It's a well-established convention of UIs that toolbars are only alternate, quick-access conveniences for frequently used items.

I very often turn off the toolbar when I need maximal vertical space. Losing functionality altogether with the toolbar off would be pretty terrible.

I know the original changes were already reverted, so this comment is mostly to record my feedback on the topic for future reference.

damianavila · 2014-01-25T16:22:43Z

@ellisonbg do you have plans to push forward this one? Thanks

ellisonbg · 2014-01-25T18:06:04Z

Yep, will finish soon.

On Sat, Jan 25, 2014 at 8:22 AM, Damián Avila notifications@github.comwrote:

@ellisonbg https://github.com/ellisonbg do you have plans to push
forward this one? Thanks

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4662#issuecomment-33292628
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

ellisonbg · 2014-01-29T03:32:40Z

Ok I have cleaned this up a bit more based on a HipChat with @minrk today. The Cell menu has the following actions for both the current and all cells (each in a submenu):

Toggle (hide/show output)
Toggle Scrolling (scroll long output)
Clear

I think this one is ready to go, but it would be good to have someone look at it again.

takluyver · 2014-01-29T04:23:14Z

While we're doing menu cleanup, can we get ids for the menu elements?
Currently, there's no good way for user JavaScript to add items to the
menus.

ellisonbg · 2014-01-29T04:24:01Z

Sure I will add those now...

ellisonbg · 2014-01-29T04:25:42Z

@takluyver to which item:

the outer dropdown menu item
the inner dropdown-menu dropdown menu

ellisonbg · 2014-01-29T04:26:59Z

I am thinking the inner one as its children are the actual menu items.

takluyver · 2014-01-29T06:05:57Z

I guess the inner one makes sense, but I have no strong preference.

takluyver · 2014-01-30T00:41:47Z

IPython/html/templates/notebook.html

-                    title="Show/Hide the output portion of a Code cell">
-                    <a href="#">Toggle Current Output</a></li>
+                <li id="current_outputs" class="dropdown-submenu"><a href="#">Current Output</a>
+                    <ul class="dropdown-menu">


Do we want ids for submenus as well? I guess you can select them as the only subelement of the menu item which opens them.

takluyver · 2014-01-30T00:45:29Z

Other than that, 👍

ellisonbg · 2014-01-30T02:13:58Z

I am actually -1 on adding ids for the submenus. With our change to
Bootstrap 3 (post 2.0) these will have to be refactored.

On Wed, Jan 29, 2014 at 4:45 PM, Thomas Kluyver notifications@github.comwrote:

Other than that, [image: 👍]

Reply to this email directly or view it on GitHubhttps://github.com//pull/4662#issuecomment-33649462
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

minrk · 2014-01-30T04:23:08Z

ids on top-level uls seems sufficient. Merging.

Menu cleanup

Carreau reviewed Dec 8, 2013
View reviewed changes

damianavila reviewed Dec 9, 2013
View reviewed changes

ghost assigned ellisonbg Dec 15, 2013

ellisonbg added 7 commits January 28, 2014 19:11

Removing Kernel menu.

d4cd13a

Add kernel restart button to toolbar.

fc7f169

Cleaning up output management in code and menus.

86f3623

Removing unneeded select prev/next from Edit menu.

2871d2c

Adding back Kernel menu and Cell Type submenu.

5a52971

Converting loops in *_all_output to $.map().

05f01a7

Simplified Cell menu items related to output.

9478cfb

Adding DOM ids for each menu.

e249650

ellisonbg mentioned this pull request Jan 29, 2014

Decouple kernel starting form notebook opening #4938

Open

takluyver reviewed Jan 30, 2014
View reviewed changes

minrk added a commit that referenced this pull request Jan 30, 2014

Merge pull request #4662 from ellisonbg/menu-cleanup

85e85ef

Menu cleanup

minrk merged commit 85e85ef into ipython:master Jan 30, 2014

ellisonbg deleted the menu-cleanup branch February 8, 2014 19:53

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#4662 from ellisonbg/menu-cleanup

ba0e009

Menu cleanup

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu cleanup #4662

Menu cleanup #4662

ellisonbg commented Dec 7, 2013

takluyver commented Dec 7, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

Carreau Dec 8, 2013

Carreau commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

damianavila Dec 9, 2013

ivanov Dec 9, 2013

damianavila Dec 9, 2013

ellisonbg Dec 9, 2013

damianavila commented Dec 9, 2013

fperez commented Dec 10, 2013

damianavila commented Jan 25, 2014

ellisonbg commented Jan 25, 2014

ellisonbg commented Jan 29, 2014

takluyver commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

takluyver commented Jan 29, 2014

takluyver Jan 30, 2014

takluyver commented Jan 30, 2014

ellisonbg commented Jan 30, 2014

minrk commented Jan 30, 2014

Menu cleanup #4662

Menu cleanup #4662

Conversation

ellisonbg commented Dec 7, 2013

takluyver commented Dec 7, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

minrk commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

Carreau Dec 8, 2013

Choose a reason for hiding this comment

Carreau commented Dec 8, 2013

ellisonbg commented Dec 8, 2013

damianavila Dec 9, 2013

Choose a reason for hiding this comment

ivanov Dec 9, 2013

Choose a reason for hiding this comment

damianavila Dec 9, 2013

Choose a reason for hiding this comment

ellisonbg Dec 9, 2013

Choose a reason for hiding this comment

damianavila commented Dec 9, 2013

fperez commented Dec 10, 2013

damianavila commented Jan 25, 2014

ellisonbg commented Jan 25, 2014

ellisonbg commented Jan 29, 2014

takluyver commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

takluyver commented Jan 29, 2014

takluyver Jan 30, 2014

Choose a reason for hiding this comment

takluyver commented Jan 30, 2014

ellisonbg commented Jan 30, 2014

minrk commented Jan 30, 2014