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

Add Hide Header which toggles header + toolbar + menubar visibility #863

Merged
merged 4 commits into from
Feb 5, 2017

Conversation

bluss
Copy link
Contributor

@bluss bluss commented Jan 29, 2017

  • Keyboard shortcut Ctrl-H toggles the visibility of the whole header (header + toolbar + menubar)

This is useful for giving talks with relatively large font size; you don't want menu bar or the status field (“checkpoint saved” etc) visible.

@jfbercher
Copy link
Member

Works and is certainly a good idea.
Looks good to me.
Can I just suggest to add a $('div#site').trigger('resize'); in the toggle_menubar function? This way, other extensions can be informed and resize themselves (eg toc2, someday..)

@jcb91
Copy link
Member

jcb91 commented Jan 30, 2017

Nice idea. To be future-proof, I'd suggest triggering the actions jupyter-notebook:toggle-header and jupyter-notebook:toggle-toolbar, rather than duplicating their functionality. This would have the added benefit of triggering the relevant resize event (resize-header.Page). However, these only appeared in version 4.1.0, so it may be worth keeping the other as a fallback for earlier versions.

@jcb91
Copy link
Member

jcb91 commented Jan 30, 2017

see notebook/static/notebook/js/actions.js#L432-L446 for the actions in notebook 4.3.0

@bluss
Copy link
Contributor Author

bluss commented Jan 30, 2017

Can I get the current on/off status of those toggles? It's needed to have everything sync up.

@bluss
Copy link
Contributor Author

bluss commented Jan 30, 2017

(Upstream should just add this as a feature, but the extension is a measure needed until then.)

@jcb91
Copy link
Member

jcb91 commented Jan 30, 2017

(Upstream should just add this as a feature, but the extension is a measure needed until then.)

agreed!

Can I get the current on/off status of those toggles?

Not through the API, as far as I've seen so far. The only way I can think of to check the current status would be to use something like

var header_shown = $('#header-container').is(':visible');
var toolbar_shown = $('div#maintoolbar').is(':visible');

which clearly isn't as future-proof, but perhaps suits the purpose?

@bluss
Copy link
Contributor Author

bluss commented Jan 30, 2017

I have no prior experience with jquery or js, so excuse all the questions. I'd like to query visibility, toggle those actions if needed, but then the toggling of the menu bar comes at the end, so the resize event still happens between say the toolbar is hidden and the menubar is (is this right?) which doesn't sound like it helps anything.

@jfbercher
Copy link
Member

Indeed, you may check for visibility before toggling. A possibility could be: If one is visible, then hide, otherwise show both. In both cases issue a (unique) resize event.
Perhaps this?

                var header_shown = $('#header-container').is(':visible');
                var toolbar_shown = $('div#maintoolbar').is(':visible');
                if (header_shown || toolbar_shown){
                    $('#header-container').hide();
                    $('#menubar-container').hide();
                }
                else{
                    $('#menubar-container').show();
                    $('#header-container').show();
                }
                events.trigger('resize-header.Page');

with var events = require('base/js/events'); or defined in the args of the define([..], ..)

@jcb91
Copy link
Member

jcb91 commented Feb 2, 2017

I have no prior experience with jquery or js, so excuse all the questions.

no worries, I make it up as I go along anyway 😉

I'd like to query visibility, toggle those actions if needed, but then the toggling of the menu bar comes at the end, so the resize event still happens between say the toolbar is hidden and the menubar is (is this right?) which doesn't sound like it helps anything.

If both actions get called, there will be a resize event for each action, so one will fire between the two, which isn't massively helpful, but is also not a problem, then the second will fire at the end, fixing everything correctly. You could limit this to one event, as @jfbercher suggests, but this adds complexity without really gaining much, so I'd just keep it simple and robust by calling the actions. Something like

var desired_shown = true;  // or false, as appropriate
if (desired_shown !== $('#header-container').is(':visible')) {
    Jupyter.keyboard_manager.actions.call('jupyter-notebook:toggle-header');
}
if (desired_shown !== $('div#maintoolbar').is(':visible')) {
    Jupyter.keyboard_manager.actions.call('jupyter-notebook:toggle-toolbar');
}

@jfbercher
Copy link
Member

A small drawback that I saw with the actions is that the menu is not hidden, reason for which I used $('#menubar-container').hide();

@jcb91
Copy link
Member

jcb91 commented Feb 2, 2017

Ah, I see, this is providing functionality not present in the default actions. Apologies, I missed that.

@bluss
Copy link
Contributor Author

bluss commented Feb 3, 2017

I'm back. I would like this to just hide/show menubar, header, toolbar as a unit. If I unhide, I want just the parts that were visible before to be visible. That maybe doesn't make much sense, but it works for me.

@jcb91
Copy link
Member

jcb91 commented Feb 3, 2017

Ugh, apologies for the misunderstanding there. Now it makes more sense, and I think your initial implementation of just hiding the whole #header div works fin. But, it's probably still worth dispensing with the css classes you've used to get the height correct in favour of just triggering the appropriate resize event:

// events should really obtained as an arg retrieved from the define([...] call
var events = require('base/js/events'); 
events.trigger('resize-header.Page');

I'd also suggest making the keyboard shortcut configurable, as ctrl-H opens browser history in many cases,so rebinding it may be undesirable for some people. That can wait for a further PR though, I guess.

@jcb91
Copy link
Member

jcb91 commented Feb 4, 2017

ah, yeah, except that by hiding the #header itself, its height is unchanged, and so the resize handler doesn't help after all. Ignore me, I think this is good to merge...

- Changed name of the extension without changing its functionality
- Add a config parameter for the hotkey
@bluss
Copy link
Contributor Author

bluss commented Feb 4, 2017

I have pushed a new commit. The mechanism of hiding the header + menubar + toolbar is still the same, just decided to rename the extension to Hide Header and the action to Toggle Header. Also added a hotkey config (with the the comment-uncomment extension as guide.

@bluss bluss changed the title Add Hide Menubar which toggles header + toolbar + menubar visibility Add Hide Header which toggles header + toolbar + menubar visibility Feb 4, 2017
@bluss
Copy link
Contributor Author

bluss commented Feb 4, 2017

Oh, there's already a default action called Toggle Header. New try for the action name is Toggle All Headers...

@jfbercher
Copy link
Member

Nice progress!
I would have prefer to hide the elements because in that case the $(''#header').height is updated, instead of "masking" the header (it is still here). But anyway it is manageable after.

Can I suggest to add a more meaningful name for the triggered event, eg events.trigger('toggle-header'); or events.trigger('toggle-all-headers'); with events added in the requirements ( 'base/js/events'), so that it is easy to catch?

@bluss
Copy link
Contributor Author

bluss commented Feb 4, 2017

Ah, the new event in commit 2 was a mistake, not intentional to keep that experiment in the code. I've changed it to a new event name.

@jfbercher
Copy link
Member

@bluss Looks good to me. Thanks.
Since it is also ok for @jcb91 I am gonna to merge. It will always be possible to update after.

@jfbercher jfbercher merged commit 3446b33 into ipython-contrib:master Feb 5, 2017
@bluss bluss deleted the hide-menubar branch February 5, 2017 17:59
@bluss
Copy link
Contributor Author

bluss commented Feb 5, 2017

Thanks a lot! I expect it will be popular.

@jcb91
Copy link
Member

jcb91 commented Feb 5, 2017

I expect it will be popular.

Quite possibly, but I'm not aware of an easy way to tell how popular any extension is, aside from the number of PRs, issues and questions it prompts. If anyone has any better ideas, I'd love to know! 😆

@oztalha
Copy link

oztalha commented Feb 14, 2017

Thanks for this, but I couldn't make it work:
pip install --upgrade https://github.com/ipython-contrib/jupyter_contrib_nbextensions/tarball/master
After installing this way, I expected Hide Header listed here, but this is what I get:

@jcb91
Copy link
Member

jcb91 commented Feb 14, 2017

@oztalha did you also run the second, nbextension install step (which copies the javascript etc into the jupyter data directories)? If you've installed using conda in the past, I'd suggest using the --sys-prefix flag (the default for conda) rather than the --user one in the instructions, to avoid having duplicate installations that the configurator will warn about.

@jcb91
Copy link
Member

jcb91 commented Feb 14, 2017

Also, just to note, this duplicates a lot of zenmode (which doesn't hide the toolbar, but does hide everything else), so it might have been easier to extend that, but it's a bit late now (my fault, should have thought to mention it earlier)!

@oztalha
Copy link

oztalha commented Feb 14, 2017

Interesting. I just reran the second step with the --debug parameter and here is what I got:

Tested, it works now, perfect. Thank you again!

PS. I'm suspecting that I first had tried to install the extensions from pypi. Then upgraded it from the git repo. But probably did not go over the 2nd step a second time then.

@jcb91
Copy link
Member

jcb91 commented Feb 14, 2017

But probably did not go over the 2nd step a second time then

That seems the most likely explanation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants