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 update: Highlight toc headings for section with selected/edited/running cells + fix save issue #795

Merged
merged 3 commits into from
Nov 24, 2016

Conversation

jfbercher
Copy link
Member

New feature

As suggested by @dinya in #791, added highlighting of the section that contains the currently edited/selected/executing cell.
Colors can be customized by changing .toc-item-highlight-select and .toc-item-highlight-execute classes in css. (actual highlight colors are gold yellow and red, but the red doen't render well in the gif anim :-< )
demo3

Fix

  • Fixed saving issue due to a race condition in loading/writing metadata; see issues #1882 and #762

- Fix "impossible to save" issue due to race condition when writing/loading notebook
metadata
@jcb91
Copy link
Member

jcb91 commented Nov 16, 2016

Nice.

Colors can be customized by changing .toc-item-highlight-select and .toc-item-highlight-execute classes in css

It's possible to edit css from javascript, allowing these to be configurable - see ruler nbextension for configurable colors, and code_font_size nbextension for js editing of css rules.

@jfbercher
Copy link
Member Author

@jcb91 Hum, that is a good idea.
May be that it would be easier in that case to directly specify the background colors (via a `.css(.,.)) and have them configurable in the yaml file.
[I will implement that in a next iteration (or welcome somebody else to do it), but I will be quite busy in the coming weeks.]

@jfbercher
Copy link
Member Author

@jcb91
Colors are now configurable!
[created a style section from javascript and populated it with css embedding values read from the config]

@jcb91
Copy link
Member

jcb91 commented Nov 23, 2016

Awesome, looks great! One question though - looks like maybe a typo using hoover instead of hover? Hoover being the vacuum-cleaner brand/US president 😉

});
function create_highlights_css() {
var sheet = document.createElement('style')
sheet.innerHTML = `#toc-level0 li > a:hover { display: block; background-color: ${cfg.colors.hoover_highlight} }
Copy link
Member

Choose a reason for hiding this comment

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

The backtick is not supported in android browser or internet explorer (see https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals#Browser_compatibility), so it might be a good idea to use a more basic string concatenation operation here, as otherwise it'll presumably be a syntax error, causing the whole nbextension to crash & burn, rather than failing mor gracefully.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it is nicer to use template strings, but indeed it might crash the extension in IE/Android. I change this to simple concatenation.

- name: toc2.colors.hoover_highlight
input_type: color
description: Hoover color in toc
default: ["#DAA520"]
Copy link
Member

Choose a reason for hiding this comment

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

This is a list of strings (because of the [...]), whereas we only want one color (string). I guess this is a hangover of my example from the ruler nbextension, which does actually use a list of colors. Sorry, not a great example!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I asked myself, but did not investigated since it worked immediately as such.

Copy link
Member

Choose a reason for hiding this comment

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

True, I'm being a bit pedantic here, since as you point out, it works regardless of spelling, as long as it's consistent.

@jcb91
Copy link
Member

jcb91 commented Nov 24, 2016

right, this looks good, I'm gonna go ahead and merge!

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