[toc2] use amd structure to avoid polluting the global namespace by jcb91 · Pull Request #1032 · ipython-contrib/jupyter_contrib_nbextensions

jcb91 · 2017-07-23T23:18:16Z

No description provided.

jcb91 · 2017-07-28T11:04:43Z

I think this should work ok, but haven't tried html export - any chance you could give it a try at some point @jfbercher? I'm thinking I'll try to move toc2 over to using the inliner class which collapsible_headings is currently using, so that we can more easily export using both of them, so this would be useful to have before attempting that :)

jfbercher · 2017-07-29T21:45:36Z

@jcb91 I have tested conversion today and apart a require error that appears in the console, it works!
That is very nice to have considered that point; Indeed I was annoyed with this pollution of the namespace. I'll recycle the idea for jupyter_latex_envs . I have pushed a small PR to your repo for that.

There is also a small backward compatibility issue to address. In old html conversions, the js tries to load directly table_of_contents(), not toc2.table_of_contents()...

jfbercher · 2017-07-29T22:59:00Z

Commit 466a78b does not address the backward compatibility issue. If we still want to support html files converted before the conversion of toc2 to a module -- which is a nice thing, I am afraid that we will have to define this module with some other name (eg toc2_lib or whatever) and keep a toc2.js that would read this module and export a table_of-contents function.
The livenotebook could work only with the module, so that namespace will not be polluted. Thoughts?

jcb91 · 2017-07-30T11:09:27Z

If we still want to support html files converted before the conversion of toc2 to a module -- which is a nice thing, I am afraid that we will have to define this module with some other name (eg toc2_lib or whatever) and keep a toc2.js that would read this module and export a table_of-contents function.
The livenotebook could work only with the module, so that namespace will not be polluted. Thoughts?

I agree, backwards compatibility is a good thing to keep. However, I don't think we need any extra modules or anything - I think we can just fix this in toc2.js using something like:

if (!liveNotebook) {
    // export main function for backwards compatibility
    window.table_of_contents = table_of_contents;
}

After all, adding just the single name table_of_contents to the global namespace in non-live notebooks isn't so bad 😉

jfbercher · 2017-07-31T16:52:58Z

@jcb91 Nice idea. I tested it but it did not worked, probably because of a race condition -- table_of_contents was not defined when reached.
I have pushed a commit where the require ensures that the library is fully loaded before using table_of_contents. In my tests, this works.

if (!liveNotebook) {
    // export main function for backwards compatibility
    function table_of_contents(cfg, st) {
        return require(['nbextensions/toc2/toc2'], function(toc2) {
            return toc2.table_of_contents(cfg, st);
        });
    }
}

jfbercher · 2017-08-07T10:52:39Z

@jcb91 For me this is working. Have you had a look to the mast commits? Do you think we can merge?

jcb91 · 2017-08-07T22:08:08Z

Apologies @jfbercher, I've been away from a computer for a few days. I'll likely get a chance to look at things wednesday. This looks to me like it'll work, but it's a bit convoluted trying to figure out what gets defined in what order, so I'd like to simplify it a little if that's ok? Anyway, feel free to merge away if it's working ok, I can always make another pr later to satisfy my ocd ;)

jfbercher · 2017-08-07T22:29:05Z

@jcb91 Sure. I have a little time myself. Simplifications will be welcome. We will merge when ready!

We use a synchronous method for detecting the live notebook, otherwise we run into various timing issues

since it's only used there anyway

…s of toc2 this is accomplished by defining a window.table_of_contents function synchronously in the first pass of the file, so that it can be called by subsequent scripts in a page.

to cater for malformed exported notebooks

jcb91 · 2017-08-23T03:38:25Z

ok, I've attempted to rebase this onto the current master, and also made a stab at simplifying the config loading. Currently I've just replicated the existing behaviour in terms of what's per-notebook and what's not, but I have a feeling some (e.g. threshold?) may have been miscatergorized. Not sure. I've also removed some unused items: the liveNotebook variable in main.js (which only gets loaded in live notebooks anyway), plus the settings config_loaded, extension_initialized, nbcontainer_marginleft, nbcontainer_marginright and nbcontainer_width, which were all unused...

jfbercher · 2017-08-23T15:36:50Z

+        nav_menu: {},
+        number_sections: true,
+        sideBar: true,
+        skip_h1_title: false,


I think that skip_1_title is a system-wide parameter.

To me, it's per-notebook, as whether it makes sense depends on the notebook content?

Yes, but we have nothing to change the behavior per notebook, except directly editing the metadata.

jfbercher

Nicely done. I have attempted something like that in #1057. Will have to rebase and merge your version :-)

jcb91 · 2017-08-23T15:40:20Z

Will have to rebase and merge your version :-)

Ah sorry, it always gets messy here! 😆

jfbercher · 2017-08-23T15:56:03Z

@jcb91 Just tested the live-version.
This works. But it seems that an error has appear here which prevents the toc to display?

jcb91 · 2017-08-23T16:06:39Z

But it seems that an error has appear here which prevents the toc to display?

Hmm, curious. It seems that draggable isn't present, from which I deduce that at toc's execution, jquery-ui hasn't yet been loaded, and indeed it doesn't appear in the page source, so I can only assume that something else (mathjax? bootstrap?) loads it later. I suppose the answer to this in a live notebook would be to require jquery-ui, but it doesn't seem to get registered correctly by the require library in the htmlpreview page, so that wouldn't work there. The only other alternative I can think of would be to just set an interval, and poll for its presence, but that seems a bit ridiculous. Any thoughts?

jfbercher · 2017-08-23T16:14:40Z

I was investigated too. Either it is jquery-ui delay for loading or a delay in inserting toc-wrapper in the DOM. In such case we would have to wait for insertion, but seems a bit complicated; or add a timeout but seems a bit ridiculous too.

jfbercher · 2017-08-23T16:25:40Z

It it were just a DOM inserting delay, we could add the .draggable() when creating the element (before insertion).

jcb91 · 2017-08-23T17:47:47Z

or a delay in inserting toc-wrapper in the DOM. In such case we would have to wait for insertion, but seems a bit complicated;

It it were just a DOM inserting delay, we could add the .draggable() when creating the element (before insertion).

Not sure I understand you here?

jfbercher · 2017-08-23T18:29:21Z

I meant that things happens as
a) var toc_wrapper = $('<div id="toc-wrapper"/>')
.append( ....
b) $("body").append(toc_wrapper);
c) $('#toc-wrapper').draggable({...
Perhaps that in c), $('#toc-wrapper') doesn't exist yet?
Since it worked previously and still works in other situations than the example reported above leaves the hypothesis opened.

jcb91 · 2017-08-23T18:32:24Z

Ah, ok. Well, from a breakpoint set at the draggable call, I see that when it happens, $.ui is still undefined, so I think at the very least we have 2 problems! 😅

jfbercher · 2017-08-23T18:39:46Z

You should be right. Indeed, the fact that the target is not defined does not rise the error $(...).draggable is not a function.

jcb91 force-pushed the tttoc branch from 9ba6fa7 to e374549 Compare July 24, 2017 02:47

jfbercher force-pushed the tttoc branch 2 times, most recently from 26cfd09 to 69b6431 Compare July 31, 2017 16:45

jfbercher force-pushed the tttoc branch from 69b6431 to 11f6be3 Compare July 31, 2017 20:43

jcb91 added 5 commits August 23, 2017 02:09

[toc2] use amd structure to avoid polluting the global namespace

37e1099

We use a synchronous method for detecting the live notebook, otherwise we run into various timing issues

[toc2] move addSaveAsWithToc definition into main.js

fe69131

since it's only used there anyway

[toc2] add backwards-compatibility for html exported with old version…

7e2baf7

…s of toc2 this is accomplished by defining a window.table_of_contents function synchronously in the first pass of the file, so that it can be called by subsequent scripts in a page.

[toc2] add extra check for cfg.cell_toc

a631fa3

to cater for malformed exported notebooks

[toc2] simplify config loading

2eb8186

jcb91 force-pushed the tttoc branch from e43e7a2 to 2eb8186 Compare August 23, 2017 03:30

jcb91 merged commit 4c4e4c6 into ipython-contrib:master Aug 23, 2017

jfbercher reviewed Aug 23, 2017

View reviewed changes

jfbercher mentioned this pull request Aug 25, 2017

toc2 export to html issue #1065

Closed

pyup-bot mentioned this pull request Oct 30, 2017

Pin jupyter_contrib_nbextensions to latest version 0.3.3 zenotech/zCFDSuperBuild#99

Merged

jcb91 deleted the tttoc branch November 28, 2017 01:14

Conversation

jcb91 commented Jul 23, 2017

Uh oh!

jcb91 commented Jul 28, 2017

Uh oh!

jfbercher commented Jul 29, 2017

Uh oh!

jfbercher commented Jul 29, 2017

Uh oh!

jcb91 commented Jul 30, 2017

Uh oh!

jfbercher commented Jul 31, 2017

Uh oh!

jfbercher commented Aug 7, 2017

Uh oh!

jcb91 commented Aug 7, 2017

Uh oh!

jfbercher commented Aug 7, 2017

Uh oh!

jcb91 commented Aug 23, 2017

Uh oh!

jfbercher Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

jcb91 Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

jfbercher Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

jfbercher left a comment

Choose a reason for hiding this comment

Uh oh!

jcb91 commented Aug 23, 2017

Uh oh!

jfbercher commented Aug 23, 2017

Uh oh!

jcb91 commented Aug 23, 2017

Uh oh!

jfbercher commented Aug 23, 2017

Uh oh!

jfbercher commented Aug 23, 2017

Uh oh!

jcb91 commented Aug 23, 2017

Uh oh!

jfbercher commented Aug 23, 2017

Uh oh!

jcb91 commented Aug 23, 2017

Uh oh!

jfbercher commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants