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

Adding color support to chalkboard #567

Merged
merged 3 commits into from
Oct 25, 2020
Merged

Adding color support to chalkboard #567

merged 3 commits into from
Oct 25, 2020

Conversation

nopid
Copy link

@nopid nopid commented Oct 24, 2020

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.

@@ -33,16 +33,17 @@
],
"scripts": {
"build": "for target in copy patch; do npm run $target; done",
"copy": "mkdir -p export; for dep in reveal.js reveal.js-chalkboard; do cp -r ./node_modules/$dep/ ./export/$dep/; done",
"patch": "for target in patch-reveal-css patch-notes patch-themes ; do npm run $target; done",
"copy": "mkdir -p export; for dep in reveal.js; do cp -r ./node_modules/$dep/ ./export/$dep/; done; cp -r ./node_modules/reveal.js-plugins/chalkboard ./export/reveal.js-chalkboard/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

the for loop is no longer needed now then

"clean": "rm -rf node_modules export"
},
"dependencies": {
"reveal.js": "~3.8.0",
"reveal.js-chalkboard": "~1.0.0"
"reveal.js": "~3.9.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like 3.9.2 is out as well, is it intended to specify 3.9.0 ?
the tests that I have just made brought me 3.9.2 anyways, is that expected ?

Copy link
Author

Choose a reason for hiding this comment

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

3.9.2 is fine, I'll fix package.json

"reveal.js": "~3.8.0",
"reveal.js-chalkboard": "~1.0.0"
"reveal.js": "~3.9.0",
"reveal.js-plugins": "rajgoel/reveal.js-plugins#3.9.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment on the version number

Copy link
Author

Choose a reason for hiding this comment

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

for reval.js-plugins the tag really is 3.9.0 on the git repository (no npm package so we fetch from git directly)

"reveal.js": "~3.8.0",
"reveal.js-chalkboard": "~1.0.0"
"reveal.js": "~3.9.0",
"reveal.js-plugins": "rajgoel/reveal.js-plugins#3.9.0"
},
"version": "380.0.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

you'd need to change this as well; 380 was for 3.8.0

Copy link
Author

Choose a reason for hiding this comment

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

going for 390.0.1 then

@parmentelat
Copy link
Collaborator

parmentelat commented Oct 24, 2020

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
Copy link
Collaborator

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
Copy link
Author

nopid commented Oct 24, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator


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
Copy link
Collaborator

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
Copy link
Author

nopid commented Oct 24, 2020

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

@nopid
Copy link
Author

nopid commented Oct 24, 2020

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

@nopid
Copy link
Author

nopid commented Oct 24, 2020

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

Two features than can be disabled by default :

  • 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 chalkboard annotations are readOnly once you move to another slide, this can be changed in the configuration of the plugin, I am not sure it should be the default behavior for RISE

@parmentelat
Copy link
Collaborator

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
Copy link
Collaborator

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

@nopid
Copy link
Author

nopid commented Oct 25, 2020

Oh I see, the 2s timeout was added in 784e7d2 :

if (! complete_config.show_buttons_on_startup) {
/* safer, and nicer too, to wait for reveal extensions to start */
setTimeout(toggleAllRiseButtons, 2000);
}

The behavior I observe is the buttons being there in the first place and being toggled (and thus disappearing) after 2s.

@parmentelat parmentelat merged commit aadaa71 into damianavila:master Oct 25, 2020
@parmentelat
Copy link
Collaborator

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
Copy link
Author

nopid commented Oct 25, 2020

Thank you for the very reactive merge!

@damianavila
Copy link
Owner

This is super @nopid! Thanks!

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.

None yet

3 participants