Zenmode Optional Header and Menubar by soamaven · Pull Request #1062 · ipython-contrib/jupyter_contrib_nbextensions

soamaven · 2017-08-23T22:59:10Z

I tweaked the Zenmode extension to be able to show the header and menubar. I was getting annoyed from having to exit Zenmode everytime I wanted to access the menubar, so I thought I'd make it an option. Did the same for header because it was easy.

Also, from everything I can tell this extension is 5.x compatible.

jcb91 · 2017-08-26T10:08:39Z

Mainly looks reasonable, but should it also affect the unhiding behaviour? Perhaps just restoring them to their pre-zen state? Perhaps I'm overthinking this though...

soamaven · 2017-08-26T17:47:48Z

I don't understand. All the js should do is toggle the hidden flag on the header and menubar. Can you elaborate on what you mean affect the unhiding behaviour and restoring their pre-zen state? This is to keep the nice background of zen while having easy access to the menubar (for things like 'close and halt')

jcb91 · 2017-08-26T18:55:38Z

Well, as it currently stands, the modified version now only hides the menubar/header as set by the config. However, when zen is toggled off again, it will unhide both again (circa line 73), regardless of what state they started in. This could be annoying if, for example, one wished zen to just not affect the menubar at all, and set that independently. Perhaps this level of detail is unimportant though, since in that case I'd have to have been prepared to manually hide/unhide the menubar anyway!

jcb91 · 2017-08-26T18:56:37Z

Anyway, I'm happy to merge as-is, but you'll need a quick release/merge to handle the recent change to config loading 😄

soamaven · 2017-08-26T21:19:42Z

However, when zen is toggled off again, it will unhide both again (circa line 73), regardless of what state they started in.

I understand what you mean. That could get annoying if I had header hidden, went zen, and then back out of zen to find the header. I unfortunately don't know how to fix this, but this is how Zenmode operates currently anyway, so it's not decreasing functionality.

The new config loading breaks the functionality... I am not sure what's going on. Before I just mimicked the config loading of the other options, and it worked fine.

jcb91 · 2017-08-26T22:24:04Z

this is how Zenmode operates currently anyway, so it's not decreasing functionality

True, I was being overly picky, this can wait till somebody reports being inconvenienced by it.

The new config loading breaks the functionality... I am not sure what's going on. Before I just mimicked the config loading of the other options, and it worked fine.

Ah... do you mean that the new loading breaks all functionality, or just the bit you've added? Could you push your merged/rebased branch so I can take a look?

…bextensions into zenheadmenu

soamaven · 2017-08-30T20:27:11Z

or just the bit you've added?

Yeah just the added bits

Could you push your merged/rebased branch so I can take a look?

Done.

jcb91

So it looks like rather than merging, you've essentially just gone back to the version before the new config loading mechanism - see individual comments for further details. I can't see an obvious reason why the new behaviour would be broken, but let's get the merge sorted first before we drill down into that!

jcb91 · 2017-08-30T22:13:49Z

-    "base/js/events"
+    "base/js/events",
+    'base/js/utils',
+    'services/config',


None of this hunk is needed, this was changed for the new config loading mechanism

Apologies, when I said "None of this hunk is needed", I meant "none of the changes in this hunk", we still need the events module 😅

jcb91 · 2017-08-30T22:14:14Z

-    events
+    events,
+    utils,
+    configmod


Also not needed (see above)

jcb91 · 2017-08-30T22:14:59Z


+    var base_url = utils.get_body_data("baseUrl");
+    var config = new configmod.ConfigSection('notebook', {base_url: base_url});
+


Also unnecessary now we have the new loading mechanism

jcb91 · 2017-08-30T22:15:38Z

            );
        }
-    };
+    });


This change isn't needed.

jcb91 · 2017-08-30T22:15:57Z

        );
        $("#maintoolbar-container").prepend($('#zenmode-btn-grp'));
-        return IPython.notebook.config.loaded.then(initialize);
+        config.load();


This change should be reverted

jcb91 · 2017-08-30T22:16:35Z


-    var initialize = function () {
-    	var config = IPython.notebook.config;
+    config.loaded.then(function() {


This (old lines 114,115 replaced by new 123) needs reverting

jcb91 · 2017-08-30T22:27:18Z

+                hide_menubar = false;
+            }
+        }
+


This bit (new lines 124-137) is still fine :)

soamaven · 2017-09-04T02:10:31Z

So it looks like rather than merging, you've essentially just gone back to the version before the new config loading mechanism - see individual comments for further details. I can't see an obvious reason why the new behaviour would be broken, but let's get the merge sorted first before we drill down into that!

Sorry. Self taught here. Git is easy to use if you stay in the shallows, but the drop-off is steep. I understand the request now.

So I rebased onto the latest commit. I added in the options, and those can be seen in the configurator. This happens due to the yaml file being correct, so that's not surprising.

The main.js though, doesn't work. None of the options are being implemented, including older ones such as "Set zenmode on when a notebook opens". Going to try to hunt down a bug in the new loader, that seems likely to be the issue, no? Merge pushed according to your comments.

Working now! Pushed commit. Should be as desired.

Also, where can i find the debug log? Reinstalling the extension with --debug isn't giving anything to the console. Starting up with jupyter notebook --debug doesn't give much information either. I imagine this would have saved me some time.

…nfig loader.

# Conflicts: # src/jupyter_contrib_nbextensions/nbextensions/zenmode/main.js

…bextensions into zenheadmenu

jcb91 · 2017-09-04T09:21:54Z

    require,
    $,
    IPython,
-    events


The events object is needed in new line 107, so this message to go back in. Everything else looks good :)

jcb91 · 2017-09-04T09:22:57Z

Sorry. Self taught here. Git is easy to use if you stay in the shallows, but the drop-off is steep. I understand the request now.

No worries! Definitely, my apologies, I probably should have provided a better explanation of what I meant!

Also, where can i find the debug log? Reinstalling the extension with --debug isn't giving anything to the console. Starting up with jupyter notebook --debug doesn't give much information either. I imagine this would have saved me some time.

So, for the JavaScript nbextensions, any useful logs will be in the browser's
javascript console. Sometimes you can also get useful information from the server logs when running jupyter notebook --debug, such as which is & CSS files are being requested by the browser.

Thanks for your persistence and patience on this! 😄

…bextensions into zenheadmenu

jcb91 · 2017-09-10T14:53:31Z

@soamaven apologies for the delay looking at this, but anyway, it looks good now! 👍

Added optional settings to hide/show header and menubar in zenmode.

7963da3

Trying jcb91's changes.

475c3dc

soamaven added 2 commits August 30, 2017 11:49

Merge branch 'master' of github.com:ipython-contrib/jupyter_contrib_n…

47d9411

…bextensions into zenheadmenu

rebase onto master and revert zenmode/main.js

3dcd8ab

jcb91 requested changes Aug 30, 2017

View reviewed changes

soamaven added 2 commits September 3, 2017 17:55

Added optional settings to hide/show header and menubar in zenmode.

a962047

rebase onto master and revert zenmode/main.js

f5eaa91

soamaven added 3 commits September 3, 2017 19:31

Merged upstream/master with zenheadmode to include the new Ipython co…

79c9bef

…nfig loader.

Merge remote-tracking branch 'origin/zenheadmenu' into zenheadmenu

e86263f

# Conflicts: # src/jupyter_contrib_nbextensions/nbextensions/zenmode/main.js

Merge branch 'master' of github.com:ipython-contrib/jupyter_contrib_n…

b721016

…bextensions into zenheadmenu

jcb91 reviewed Sep 4, 2017

View reviewed changes

jcb91 approved these changes Sep 4, 2017

View reviewed changes

soamaven added 6 commits September 4, 2017 09:05

Merge branch 'master' of github.com:ipython-contrib/jupyter_contrib_n…

d29c75b

…bextensions into zenheadmenu

Put events object back in

3b9bf7f

Put events object back in

5ad1ab4

Merge remote-tracking branch 'origin/zenheadmenu' into zenheadmenu

16582eb

Merge remote-tracking branch 'origin/zenheadmenu' into zenheadmenu

2b62c1c

Merge remote-tracking branch 'origin/zenheadmenu' into zenheadmenu

c0d49b2

jcb91 merged commit 5f97333 into ipython-contrib:master Sep 10, 2017


		var base_url = utils.get_body_data("baseUrl");
		var config = new configmod.ConfigSection('notebook', {base_url: base_url});

Conversation

soamaven commented Aug 23, 2017

Uh oh!

jcb91 commented Aug 26, 2017

Uh oh!

soamaven commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcb91 commented Aug 26, 2017

Uh oh!

jcb91 commented Aug 26, 2017

Uh oh!

soamaven commented Aug 26, 2017

Uh oh!

jcb91 commented Aug 26, 2017

Uh oh!

soamaven commented Aug 30, 2017

Uh oh!

jcb91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soamaven commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcb91 commented Sep 4, 2017

Uh oh!

jcb91 commented Sep 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

soamaven commented Aug 26, 2017 •

edited

Loading

soamaven commented Sep 4, 2017 •

edited

Loading