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

[toc2] Scrolling: add a mark to currently displayed section in TOC window (including @jcb91 toc2 amd module) #1068

Merged
merged 3 commits into from
Sep 12, 2017

Conversation

jfbercher
Copy link
Member

This adds support for displaying a small mark at the left of the toc item corresponding to the first header displayed in notebook window. The mark moves as the notebook is scrolled. This was an enhancement request in #944.
Two parameters have been added to the configuration: one for enabling this behavior, the second for altering the color of the mark.
Export have been adapted to preserve this scrolling indicator when exporting the notebook to html.
demo_scroll_collapse

Other modifications include

  • a small rework of configuration load
  • colors are defined in css in addition to being adjusted in nbextension-configurator, so as to present decent defaults when exporting to html.

@jfbercher
Copy link
Member Author

For those interested in, this can be easily tested by

jupyter nbextension install https://rawgit.com/jfbercher/small_nbextensions/master/toc2.zip  --user

(assuming toc2 is already enabled and installed as user/ otherwise use --system instead of --user).

Copy link
Member

@jcb91 jcb91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments inline. The other main thing is that I think it would be preferable not to include a 5Mb(!) animated gif, given the already-large (41.3Mb!) size of our source distribution. Just a couple of stills would be enough to get the idea across, no?

// read configuration, then call toc
cfg = read_config(cfg, function() {
table_of_contents(cfg, st);
highlightTocItemOnScroll(cfg, st);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think highlightTocItemOnScroll should just be called from table_of_contents directly, then it doesn't need importing in main, or calling in template!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did it like that is that it should be called a single time (and table_of_contents is called each time a md cell is rendered). I use now the test in table_of_contents which checks if the toc-wrapper exists and if not create it (call create_toc_div). In such a case, we are in an initialization step for the toc and the highlightTocItemOnScroll can also be called. This has the interest to also work in the non-livenotebook (after html export) version.

@@ -9,6 +9,8 @@
var IPython;
var events;
var liveNotebook = false;
var all_headers= $("#notebook").find(":header");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you do this here, rather than in the table_of_contents function (or the new highlight.. function), then I think you only pick up headers that were extant when the nbextension loaded, which is not the right behaviour...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only serves to initialize all_headers. They are updated in table_of_contents. This was needed to let highlightTocItemOnScrollhave access to all_headers.

@@ -7,7 +7,7 @@ define([
'jquery',
'base/js/namespace',
'notebook/js/codecell',
'./toc2'
'nbextensions/toc2/toc2'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relative path is less confusing here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update this one. Not sure it is less confusing, anyway.

@@ -21,11 +21,13 @@ define([
var highlight_toc_item = toc2.highlight_toc_item;
var table_of_contents = toc2.table_of_contents;
var toggle_toc = toc2.toggle_toc;
var highlightTocItemOnScroll = toc2.highlightTocItemOnScroll;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this import is unnecessary if highlightTocItemOnScroll is just automatically called from table_of_contents


function create_additional_css() {
var sheet = document.createElement('style')
sheet.innerHTML = "#toc-level0 li > span:hover { background-color: " + cfg.colors.hover_highlight + " }\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was supposed to be span:hover, but it's got changed to a:hover?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch. updated.

@@ -17,8 +19,8 @@
liveNotebook = true;
}
catch (err) {
// log the error, just in case we *are* in a live notebook
console.log('[toc2] working in non-live notebook:', err);
// We *are* in a live notebook
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This altered comment is incorrect. If here, we seem not to be in a live notebook, but write log the error for debugging in case it turns out we've got the wrong impression, and actually are in a live notebook. I think keeping the error in the log makes sense, unless it looks confusingly like an actual error, in which case it might benefit from some clarification?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a typo. I have removed the print of the err because it is printed in the non-livenotebook version, as it should, but this may suggest to the user that there is a problem..

// (if displayed as a sidebar)
if (liveNotebook) {
$([Jupyter.events]).on("resize-header.Page", function() {setSideBarHeight(cfg, st);});
$([Jupyter.events]).on("toggle-all-headers", function() {setSideBarHeight(cfg, st);});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be made in a single call

 $([Jupyter.events]).on("resize-header.Page toggle-all-headers", function() {setSideBarHeight(cfg, st);});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. thanks.

st.oldTocHeight = undefined
st.cell_toc = undefined;
st.toc_index=0;

// fire the main function with these parameters
require(['nbextensions/toc2/toc2'], function (toc2) {
toc2.table_of_contents(cfg, st);
toc2.highlightTocItemOnScroll(cfg,st);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed, see above

@jfbercher jfbercher merged commit fef2697 into ipython-contrib:master Sep 12, 2017
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.

2 participants