damianavila / RISE

RISE: "Live" Reveal.js Jupyter/IPython Slideshow Extension
Other
3.67k stars 414 forks source link

Adding color support to chalkboard #567

Closed nopid closed 3 years ago

nopid commented 3 years ago

reveal.js-plugins added color support to chalkboard some time ago.

This PR moves reveal.js from 3.8.0 to 3.9.0 and imports chalkboard from the last stable v3 reveal-js.plugins Two keys are binded to color cycling : I choose Q & S on my AZERTY keyboard but there are certainly better options.

parmentelat commented 3 years ago

hi; thanks for this PR !

I made a few comments above on the code

in addition, I am concerned because during my tests of our code there were a few features that would no longer work like, EDIT (see below) the X and ? buttons are gone, as well as the chalkboard buttons (but on this one I'm not quite sure if I was looking at a slideshow that had them enabled, so..) and the 't' keyboard shortcut no longer gives me the notes (but this one might have been broken earlier)

so, last time we upgraded reveal.js, we had a few more tweaks to do around.. did you test this version yourself ? now, it might also very well be that I screwed up when installing that devel version


more general question out of curiosity: what about release 4.x ? have you invested any time looking into that ?

thanks again !

parmentelat commented 3 years ago

important edit

I just realized I was running my tests with a previous feature (#561) enabled as a leftover of my environment, that is why I was not getting the buttons !!

so it seems that only the 't' keyboard shortcut is not working, which again might very well be an earlier regression

nopid commented 3 years ago

Hi, thanks for your comments. I will check and fix the version numbers.

I did it the other way around: made a quick hack fix to add color in a presentation two days ago and tried to propose a PR. The 't' shortcut was definitely working during my talk so maybe when I cleaned the code I forgot something.

The git of the talk is right here (with the .tar.gz hack :P) : https://gogit.univ-orleans.fr/lifo/no/quantum/

parmentelat commented 3 years ago

The 't' shortcut was definitely working during my talk so maybe when I cleaned the code I forgot something.

or maybe I'm the one who has something altered in my environment; I don't work on this often enough…

besides, I'm still toying around with it; the multiple colours on the chalkboard are cooool ! THANK YOU :) I haven't seen the 2 new keystrokes documented in the ? help though, would make sense to add them there

parmentelat commented 3 years ago

I just released the master branch as official 5.7.0; this is something that was long overdue anyway

with that in place, I'll be able to publish your pr once merged as 5.7.1-dev0 so it will be available through pip install --pre rise

parmentelat commented 3 years ago

asking again in case you missed it :

more general question out of curiosity: what about release 4.x ? have you invested any time looking into that ?

nopid commented 3 years ago

I haven't looked at 4.x right now. I ported color while preparing a talk. No idea if it breaks something else.

nopid commented 3 years ago

About the notes being broken, it is documented into your commit 987a9bfa18c08c888e3bde018fc5e4fbbe45f238

nopid commented 3 years ago

Ok, I fixed the notes issue : now pressing 't' brings the notes window.

Two features than can be disabled by default :

parmentelat commented 3 years ago

the X and ? buttons are only displayed for 2s before they hide (and , bring them back), it might be short for a new user ;

the intention was to keep the buttons by default, and to let people opt out through configuration (hence the relatively short duration) are you saying that hiding the buttons is the default behaviour ? if so, we have a bug

parmentelat commented 3 years ago

thanks for fixing the notes, and for refreshing my memories; as I said I don't work on this often enough;)

nopid commented 3 years ago

Oh I see, the 2s timeout was added in 784e7d234e13f6344289be799bb8af4518848a8f : https://github.com/damianavila/RISE/blob/67158fc2804e64626efd1ebc857159459e9cafa6/classic/rise/static/main.js#L659-L662 The behavior I observe is the buttons being there in the first place and being toggled (and thus disappearing) after 2s.

parmentelat commented 3 years ago

looks good to me, merged

I issued a 5.7.0-dev0 that contains this PR as well as a fix for the buttons thingy, which was indeed broken

thanks for all, that is very helpful and much appreciated

nopid commented 3 years ago

Thank you for the very reactive merge!

damianavila commented 3 years ago

This is super @nopid! Thanks!