Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding toggle line numbers for all cells #1312

Merged
merged 1 commit into from Aug 16, 2016

Conversation

@vivi
Copy link
Contributor

@vivi vivi commented Apr 7, 2016

Hi! I'm a research apprentice at BIDS with Matthias (@ Carreau). This is for #1244.

Creates a button in the main toolbar that will toggle the line numbers on/off in all code cells. Future PR will have line numbering persist (i.e. new code cells will have line numbers turned on, if the user turned on line numbers for all cells).

toggle-button

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Apr 7, 2016

Great, thanks for the contribution. Could you post a screenshot showing the
design of the button in the toolbar to help in its review?

On Wed, Apr 6, 2016 at 5:18 PM, Vivian Fang notifications@github.com
wrote:

Hi! I'm a research apprentice at BIDS with Matthias (@ Carreau). This is
for #1244 #1244.

Creates a button in the main toolbar that will toggle the line numbers
on/off in all code cells. Future PR will have line numbering persist (i.e.
new code cells will have line numbers turned on, if the user turned on line

numbers for all cells).

You can view, comment on, or merge this pull request online at:

#1312
Commit Summary

  • adding an action for toggling all line numbers, with keyboard
    shortcut Shift-L

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1312

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

@minrk minrk commented Apr 7, 2016

Great! It seems to me like this should go in the View menu, rather than the toolbar, though, given its relatively rare frequency.

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 7, 2016

Great, thanks for the contribution. Could you post a screenshot showing the
design of the button in the toolbar to help in its review?

Screenshot added on first comment.

@@ -65,6 +65,7 @@ define(function (require) {
this.ws_url = options.ws_url;
this._session_starting = false;
this.last_modified = null;
this.line_numbers = false;

This comment has been minimized.

@Carreau

Carreau Apr 7, 2016
Member

I think I would make that private with a leading underscore, so that we can change the implementation easily.

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 7, 2016

Do we want to make the "all lines" numbers a property so that you can easily implement not a only a toggle, but a "show all" and "hide all".

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Apr 7, 2016

I tend to agree with @minrk that this is used infrequently and should probably be in the View menu. At the same time it doesn't take up much space in the toolbar.

But, in either case, it should be included in the View menu (even if in the toolbar as well)

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 7, 2016

I tend to agree with @minrk that this [...] should probably be in the View menu.

Sure, I was just showing @Secant how to do thing, we did not get to the point of adding it in the menu.

[...] this is used infrequently [...]

Well... it is used infrequently...

  1. There is no UI to do it
  2. it was practically speaking not possible to do it.

It's like arguing that setting you car is self driving mode is not such a useful feature an as a proof you show that the number of car that self-drive on the road is low.

Also, it also depend on whether we persist the value in nbconfig or not. If we persist it, people will use it once and forget it. At which point I agree that the toolbar is likely not necessary. If we don't persist it and keep it as an instance variable. some users will likely use it every time they open the notebook.

Lastly, It was one of the most requested feature at JupyterDays, which make me think that – in the case we don't persist it to nbconfig – a visible UI element for it seem likely useful.

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 7, 2016

I think that in the long run, a menu entry and persistence is the right way to do this. Whether we want to put it in the menu first and trust that we get round to persistence, or put it in the toolbar until we do persistence, I'm not sure.

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 7, 2016

BTW, just for @Secant information, the View menu is generated server side in notebook/templates/notebook.html

So you likely want to create a new menu like that

diff --git a/notebook/templates/notebook.html b/notebook/templates/notebook.html
index 10e6095..d9f5329 100644
--- a/notebook/templates/notebook.html
+++ b/notebook/templates/notebook.html
@@ -162,6 +162,10 @@ data-notebook-path="{{notebook_path | urlencode}}"
                             <a href="#">Cell Toolbar</a>
                             <ul class="dropdown-menu" id="menu-cell-toolbar-submenu"></ul>
                         </li>
+                        <li class="divider"></li>
+                        <li id="jupyter-notebook-toggle-line-number-or-any-other-id-you-like">
+                            <a href="#">Toggle Line Number</a>
+                        </li>
                     </ul>
                 </li>

and you probably figured out on yourself that you have to modify id_actions_dict in menubar.js to bind the menu with the action id.

Hope that helps.

Feel free to push any more commit on the same branch and the PR will update.
You can also ask question, I think anyone in this thread will be happy to help you.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Apr 8, 2016

I like the idea of adding it to the View menu and making it persistent.
Then users can start to use it and we can see if it is userd enough to be
on the toolbar.

On Thu, Apr 7, 2016 at 4:07 PM, Matthias Bussonnier <
notifications@github.com> wrote:

BTW, just for @Secant https://github.com/secant information, the View
menu is generated server side in notebook/templates/notebook.html
https://github.com/jupyter/notebook/blob/master/notebook/templates/notebook.html

So you likely want to create a new menu like that

diff --git a/notebook/templates/notebook.html b/notebook/templates/notebook.html
index 10e6095..d9f5329 100644
--- a/notebook/templates/notebook.html
+++ b/notebook/templates/notebook.html
@@ -162,6 +162,10 @@ data-notebook-path="{{notebook_path | urlencode}}"
Cell Toolbar


    •                    <li class="divider"></li>
      
    •                    <li id="jupyter-notebook-toggle-line-number-or-any-other-id-you-like">
      
    •                        <a href="#">Toggle Line Number</a>
      
    •                    </li>
                   </ul>
               </li>
      

    and you probably figured out on yourself that you have to modify
    id_actions_dict in menubar.js to bind the menu with the action id.

    Hope that helps.

    Feel free to push any more commit on the same branch and the PR will
    update.
    You can also ask question, I think anyone in this thread will be happy to
    help you.


    You are receiving this because you were mentioned.
    Reply to this email directly or view it on GitHub
    #1312 (comment)

    Brian E. Granger
    Associate Professor of Physics and Data Science
    Cal Poly State University, San Luis Obispo
    @ellisonbg on Twitter and GitHub
    bgranger@calpoly.edu and ellisonbg@gmail.com

    @minrk minrk added this to the 5.0 milestone Apr 8, 2016
    @SylvainCorlay
    Copy link
    Member

    @SylvainCorlay SylvainCorlay commented Apr 10, 2016

    👍

    @Carreau Carreau self-assigned this Jun 4, 2016
    @Carreau Carreau modified the milestones: 4.3, 5.0 Jul 29, 2016
    @gnestor
    Copy link
    Contributor

    @gnestor gnestor commented Aug 11, 2016

    Reviving this one... I moved the toggle line numbers from the toolbar to the "View" menu. See #1676.

    @ellisonbg Any UX feedback?

    demo

    @ellisonbg ellisonbg merged commit 21bbe1d into jupyter:master Aug 16, 2016
    1 check passed
    1 check passed
    continuous-integration/travis-ci/pr The Travis CI build passed
    Details
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Linked issues

    Successfully merging this pull request may close these issues.

    None yet

    7 participants
    You can’t perform that action at this time.