Skip to content

Ability to build toolbar in JS #2127

Merged
merged 13 commits into from Oct 2, 2012

6 participants

@Carreau
IPython member
Carreau commented Jul 12, 2012

Description:

This allow to easily create new toolbar/append buttons on existing one purely in JS.
This is done by passing dict of icon , label , callback [,id] to a methods of toolbar.

Details:

I changed the current ToolBar implementation to use this, and named it MainToolBar which inherit of IPython.ToolBar. The only instance is now named IPython.maintoolbar.
IPython.ToolBar is more general and can be use as a template to create any toolbar with buttons.
I removed the toolbar content from the html template and left only the <div id='toolbar'>

code example :

maintoolbar.addButtonsGroup([
            {
                'label'   : 'Insert Cell Above',
                'icon'    : 'ui-icon-arrowthickstop-1-n',
                'callback': IPython.notebook.insert_cell_above
            },
            {
                'label'   : 'Insert Cell Below',
                'icon'    : 'ui-icon-arrowthickstop-1-s',
                'callback': IPython.notebook.insert_cell_below
            }
        ]);

Demo

I made a quick demo of what can be done by copy pasting this js file (gist) into a MD cell:

Notice the slideshow icon on the right of markdown that is inserted if you uses the above script
Imgur
( CSS looks broken, but it's chrome dev, master have same issues)

And what it look like when clicked. One can get back to normal view by clicking the stop button or go to next/prev slide. One cell == one slide , and the notebook is still fully functional, cell can be edited and run live.
Imgur

Carreau added some commits Jun 22, 2012
@Carreau Carreau Allow toolbar construction in js
Base of allowing a full toolbar construction in js without altering the
html template, and use it to construct our toolbar as example.

still need some work to be totally decoupled.
31a6158
@Carreau Carreau add maintoolbar file e45a24b
@Carreau Carreau load maintoolbar.js dd25e33
@Carreau Carreau correcty inherign the main toolbar 4fd138f
@Carreau Carreau dont use string as dict key, better redability a6b6d75
@damianavila

This is great, I like the idea of a live presentation (vIPer only provided static presentations). Maybe it would be great to have more that one cell per slide because you have more power to build the content (maybe a new type or subtype of cell, in vIPer I use markdown and (----) to separate slides). BTW, I really enjoy the fact of a native solution for this issue.

@Carreau
IPython member
Carreau commented Jul 13, 2012

The slideshow demo is not the goal of this PR. But a demo of what can be done.

Is SO want to take over, there is a lot to add like displaying cell/output separately... etc.
And we could store data in cell metadata attributes.

But first let's see if we can merge this PR !

Please fork the gist and have fun !

@damianavila

I have to get some insight with JS first, but I will play with that for sure.

Thanks.

@Carreau
IPython member
Carreau commented Jul 23, 2012

anybody ?
You saw what it can do when building js-extension at SciPy.
WIll merge in 48h unless SO have an issue with it.

@fperez
IPython member
fperez commented Jul 24, 2012

Hold tight: while I love the idea, there's enough new JS code in here that it really should get a real review by someone else. While we need to keep the PR queue moving, and I realize it's a bummer that we're so rate-limited with our small team, we need to be a little patient with larger PRs; sometimes just pinging others and giving them a couple of extra days is enough to get good feedback.

@ellisonbg, if you have a minute to look at this, it would be fantastic. I should say I'm totally +1 on the idea, which proved its value at Scipy, but given it touches so much JS you originally wrote, you might have some feedback before merge...

@ellisonbg
IPython member
@ellisonbg
IPython member

Does #2193 replace this? What is their relationship?

@Carreau
IPython member
Carreau commented Jul 27, 2012

Yes, there is a relationship.
This one does not bundle anything about slideshow.
Just internal change, ability to create easily a toolbar totally in js.

#2193 is the draft I made on top of this to build slideshow extension.

@fperez github did hear you and brought back the user/branch on the headers of PR :-)

@Carreau
IPython member
Carreau commented Jul 27, 2012

fix Here the comment you made in #2193 about the CamelCase of methods names.

@Carreau
IPython member
Carreau commented Jul 31, 2012

@ellisonbg

What do you think of this one ? I'm building #2193 on top of this one, so I would prefer to merge this to build on top of master.

@ellisonbg ellisonbg commented on the diff Aug 11, 2012
IPython/frontend/html/notebook/static/css/notebook.css
@@ -60,7 +60,7 @@ span#notebook_name {
z-index: 10;
}
-#toolbar {
+.toolbar {
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

I see you have made this a class but in the main html template, it is still the toolbar id. Is this right?

@Carreau
IPython member
Carreau added a note Aug 12, 2012

yes, need the #id to pass it as the selector for the constructor.
toolbar class is added by js.
changing id to #maintoolbarfor consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/maintoolbar.js
@@ -0,0 +1,182 @@
+//----------------------------------------------------------------------------
+// Copyright (C) 2008 The IPython Development Team
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

File was created in 2011.

@Carreau
IPython member
Carreau added a note Aug 12, 2012

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/toolbar.js
}
};
+ // add a group of button into the current toolbar.
+ //
+ // First argument : Mandatory
+ // list of dict as argument, each dict should contain
+ // 3 mandatory keys and values :
+ // label : string -- the text to show on hover
+ // icon : string -- the jQuery-ui icon to add on this button
+ // callback : function -- the callback to execute on a click
+ //
+ // and optionnaly an 'id' key that is assigned to the button element
+ //
+ // Second Argument, optionnal,
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

optional

@Carreau
IPython member
Carreau added a note Aug 12, 2012

fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/toolbar.js
+ // IPython.toolbar.add_button_group([
+ // {label:'my button',
+ // icon:'ui-icon-disk',
+ // callback:function(){alert('hoho'),
+ // id : 'my_button_id', // this is optionnal
+ // }
+ // },
+ // {label:'my second button',
+ // icon:'ui-icon-scissors',
+ // callback:function(){alert('be carefull I cut')}
+ // }
+ // ],
+ // "my_button_group_id"
+ // )
+ //
+ ToolBar.prototype.add_button_group = function(list, group_id){
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

functions (list, group_id) (

@Carreau
IPython member
Carreau added a note Aug 12, 2012

fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/maintoolbar.js
+//----------------------------------------------------------------------------
+// Copyright (C) 2008 The IPython Development Team
+//
+// Distributed under the terms of the BSD License. The full license is in
+// the file COPYING, distributed as part of this software.
+//----------------------------------------------------------------------------
+
+//============================================================================
+// ToolBar
+//============================================================================
+
+var IPython = (function (IPython) {
+
+ var MainToolBar = function (selector) {
+ this.selector = selector;
+ if (this.selector !== undefined) {
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

This type of if statement only needs to be on the base classes. I would just put the code in this block at the top level and get rid of the test. See Cell and TextCell for an example. I know this is very weird.

@Carreau
IPython member
Carreau added a note Aug 12, 2012

hu... ok, hum not sure I understand.
Just the if statement, or the methods called inside ?
because this.add_drop_down_list(); might not exist exist on other subclasses...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/maintoolbar.js
+ .addClass('ui-widget ui-widget-content')
+ .append($('<option/>').attr('value','code').text('Code'))
+ .append($('<option/>').attr('value','markdown').text('Markdown'))
+ .append($('<option/>').attr('value','raw').text('Raw Text'))
+ .append($('<option/>').attr('value','heading1').text('Heading 1'))
+ .append($('<option/>').attr('value','heading2').text('Heading 2'))
+ .append($('<option/>').attr('value','heading3').text('Heading 3'))
+ .append($('<option/>').attr('value','heading4').text('Heading 4'))
+ .append($('<option/>').attr('value','heading5').text('Heading 5'))
+ .append($('<option/>').attr('value','heading6').text('Heading 6'))
+ .append($('<option/>').attr('value','heading7').text('Heading 7'))
+ .append($('<option/>').attr('value','heading8').text('Heading 8'))
+ );
+ }
+
+ MainToolBar.prototype.construct = function() {
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

I would put construct before add_drop_down_list as it is called first.

@Carreau
IPython member
Carreau added a note Aug 12, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 11, 2012
IPython/frontend/html/notebook/static/js/maintoolbar.js
+ .append($('<option/>').attr('value','code').text('Code'))
+ .append($('<option/>').attr('value','markdown').text('Markdown'))
+ .append($('<option/>').attr('value','raw').text('Raw Text'))
+ .append($('<option/>').attr('value','heading1').text('Heading 1'))
+ .append($('<option/>').attr('value','heading2').text('Heading 2'))
+ .append($('<option/>').attr('value','heading3').text('Heading 3'))
+ .append($('<option/>').attr('value','heading4').text('Heading 4'))
+ .append($('<option/>').attr('value','heading5').text('Heading 5'))
+ .append($('<option/>').attr('value','heading6').text('Heading 6'))
+ .append($('<option/>').attr('value','heading7').text('Heading 7'))
+ .append($('<option/>').attr('value','heading8').text('Heading 8'))
+ );
+ }
+
+ MainToolBar.prototype.construct = function() {
+ this.add_buttons_group([
@ellisonbg
IPython member
ellisonbg added a note Aug 11, 2012

The notebook is currently broken in this branch. Due to typo here: should be add_button_group.

@Carreau
IPython member
Carreau added a note Aug 12, 2012

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg and 1 other commented on an outdated diff Aug 12, 2012
IPython/frontend/html/notebook/static/js/maintoolbar.js
+ IPython.notebook.move_cell_up();
+ },
+ },
+ {
+ id:'move_down_b',
+ label:'Move Cell Down',
+ icon:'ui-icon-arrowthick-1-s',
+ callback:function(){
+ IPython.notebook.move_cell_down();
+ },
+ },
+ ],'move_up_down');
+
+ this.add_buttons_group([
+ {
+ id:'insert_above_b',
@ellisonbg
IPython member
ellisonbg added a note Aug 12, 2012

Code style comments. Put spaces before and after : in each of these dict elements. Spacing of callback to be function () {.

@Carreau
IPython member
Carreau added a note Aug 12, 2012

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg
IPython member

This looks like pretty solid straightforward code. Once you fix the typo that is breaking things I will do a functionality test to make sure everything is working. One other comment, the toggle method of the toolbar calls IPython.layout_manager.do_resize();. But there is a chance this code could be used outside this page where layout_manager is defined. Should we first test for its existence before calling its do_resize method?

@travisbot

This pull request passes (merged 7aefb27 into bf4aa6d).

@Carreau
IPython member
Carreau commented Aug 12, 2012

Except the test for selector where i'm not sure what to do, I should have fixed everything, including test for layout_manager.
I let you test now, and hopefully TravisBot will soon says that he is happy.

@ellisonbg
IPython member

Having second thoughts about this one. I have been looking more at bootstrap and it would be easy to transition to bootstrap if we keep the toolbar in HTML. Moving it to Javascript binds us more closely to jQuery UI. I don't think we can transition completely to bootstrap and stop using jQuery UI, but for something bootstrap would be a huge improvement.

@Carreau
IPython member
Carreau commented Aug 13, 2012

I don't see why you couldn't build the toolbar in JS using bootstrap.
Can you develop why ?

And nothing prevent us from having a jQueryUI toolbar below a bootstrap menubar, it it ?

@Carreau
IPython member
Carreau commented Aug 13, 2012

@ellisonbg,
Adding button to a toolbar in JS with bootstrap works, and is even easier, as you have less class to worry about (rounded left, right, up, down...) I just did it live by adding working button here from the JS console to the "toolbar" example.

Switching is not an easy task, and are we even sure it will work or will be done for 0.14 ?

Anyway, if we switch to bootstrap I will take care of converting this code if it is what worries you.

If you wish, I can even try to re-write this PR using Bootstrap for the toolbar to start the migration.

@Carreau
IPython member
Carreau commented Aug 13, 2012

FYI, I tried injecting bootstrap css into the notebook and build a toolbar in Js with it.

  • Js uses less lines (ok, 11 instead of 13) and is more readable.
  • There are css name conflict/inheritnaces making bootstrap change the appearance of some element.
  • Someting in our CSS/(jqueryUI CSS?) prevent the bootstrap icons to work.
  • It works.

Though, bootstrap use css3 heavily, meaning that lots of animation won't append on older browser.

@Carreau
IPython member
Carreau commented Aug 14, 2012

Most of the icon we use right now are not in bootstrap, but one can use Font-Awesome

after some css tweeking, it look like this.
Imgur
Not exactly as before but shouldn't be too hard.
See if you wan't to try.
https://github.com/Carreau/ipython/compare/jstoolbar-bootstrap

@bfroehle

I can't comment on the technical merits of the jstoolbar-bootstrap branch, but I have played around with it for a while and haven't noticed any functional issues.

In terms of aesthetics, the buttons seem too tiny (compared to the rest of the application which is relatively spacious). Removing the btn-small class helps a lot. I think there was, and will continue to be, confusion between the "move cell up/down" and "insert cell up/down" buttons. Looking through Font Awesome, the most similar to the previous icons are icon-upload-alt and icon-download-alt, but it's not clear to me that they are any more obvious.

The play icon has bugged me for a while, I think it should be a "step-foward" button to indicate that it just runs one cell and advances to the next cell.

@Carreau
IPython member
Carreau commented Aug 14, 2012

The js-bootstrap is a proof of concept to see what can be done with bootstrap. Next step would be tweak bootstrap css to have the same look as the actual.

As for the icons, we could roll our own font with
http://keyamoon.com/icomoon/app/

This was referenced Aug 23, 2012
@Carreau
IPython member
Carreau commented Aug 25, 2012

@ellisonbg
Can I merge this one, or do you want me to focus on first bringing bootstrap for notebook ?
BTW there is a new 5 day old version of bootstrap, which bring toolbar and submenu.

@Carreau
IPython member
Carreau commented Sep 28, 2012

Pinging @ellisonbg , what do you think I should do with this one ?
It is doable with the same api in bootstrap, and once the number of open PR decrease, I would like to start migrating the notebook to bootstrap.
Could I then merge it ?

@ellisonbg
IPython member

I think we can merge this now. A few things to keep in mind:

  • I am +1 on moving to bootstrap, but let's do that in a separate PR.
  • In general, I am trying to keep as much of our page structure in the HTML template as possible. IOW, I want most of the DOM to be created in HTML, not dynamically in JS. That is why I have been a bit hesitant on this one. But I understand we do need this to be resuable, so I am OK with this being in JS. But please keep this in mind in other development work.

Great work!

@ellisonbg ellisonbg merged commit d9a42fe into ipython:master Oct 2, 2012

1 check passed

Details default The Travis build passed
@Carreau
IPython member
Carreau commented Oct 2, 2012

Great ! Thanks!
Nice to have you Back !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.