chalkboard redeux #355

parmentelat · 2018-03-01T11:39:09Z

This is a redeux for #226

Many notes though:

This is not quite completely working yet, but I have to brain dump this b/c I lack more time to completely finish
this PR is built on top of the one on nbextensions making rise compliant with nbextensions_configurator #344, so it is much better to study after that one
I use that configurator thing to enable/disable this new extension, and by default right now it is turned off; this way we can integrate even if not working perfectly
In the mix, I have adopted @superbobry's change about Reveal.initialize() vs Reveal.configure() because, as he pointed out, dependencies are not applied when using the latter. And this btw has an impact on loading the notes plugin as well
I have not found a way to install the chalkboard through npm install; this needs to be double checked as I am far from an expert here. So I ship this as part of our our repo - I refrained from using git subtrees here - but in the new reveal-plugins subdir, and not in rise/static/reveal.js which is an area automatically built.
The new aggregate target npm run build is convenience that redoes everything (except npm install, and maybe I was wrong here)

The remaining issues are documented in the last commit; I have not yet reached the point where I could apply superbobry's cool idea of putting more stuff in css from js, will probably be needed later on to address the overlapping issues.
But I am getting too far apart and am running out of time, so I feel it's best to make a stop here.

damianavila · 2018-03-01T12:51:40Z

This is super @parmentelat! Thanks for doing it. Will try to provide feedback in the next few days.

parmentelat · 2018-03-02T09:35:31Z

One last note, I forgot to mention that the original PR by @superbobry introduced more separation of styling from js into css, that I liked, but have had no time to reproduce here, and that would still be worth doing IMO

damianavila · 2018-03-08T22:26:21Z

Let's finish #344 before doing a review of this one, so I do not review the same thing twice (also difficult to see the real changes because of the noise from the other PR).
But one general thing we should do is to make the chalkboard plugin and npm package and get is from npm as we do with reveal.js.

It should be straightforward to make the npm package. It should be just a matter to add the proper package.json and you are done. Let me know if you need help with this. After that we can download the chalkboard from npm and we do not need to ship additional code.

Btw, there are really interesting plugins out there... I did not have time to check them but some of them are really nice, ie the menubar plugin...

parmentelat · 2018-03-09T10:10:38Z

Agreed, I opened this issue with the original author a few days back
rajgoel/reveal.js-plugins#46

feel free to elaborate on this issue directly, it would be more optimal if done natively at the source :)

parmentelat · 2018-03-09T13:25:17Z

In addition, going back to the "let's put this in standby" arguments, I wanted to add this

Right now the code kind of works but it's supoptimal, in particular it behaves very poorly with input keyboard events that are defined both in the chalkboard and in the jupyter workd

it just occurred to me that one way to go about that problem would maybe to have these keyboard events managed by jupyter too. just an idea for now, but I'll try to give this a shot if/when I have a chance

damianavila · 2018-03-09T13:32:58Z

feel free to elaborate on this issue directly, it would be more optimal if done natively at the source :)

Thanks, will try to put some comments there if we don't see actions.

it just occurred to me that one way to go about that problem would maybe to have these keyboard events managed by jupyter too. just an idea for now, but I'll try to give this a shot if/when I have a chance

Sounds like a good idea.

parmentelat · 2018-03-23T20:21:03Z

as requested: this PR should now only show the relevant changes; hopefully at least ;)

parmentelat · 2018-03-24T09:22:06Z

As a summary / status on this one, two topics are still unsettled

I don't like the way keyboard events are managed in the current version, I need to try the other approach of having them all managed by the jupyter event loop
also, the way the chalkboard plugin code is currently distributed can be largely improved. As a reminder, for now I have duplicated this code in this repo under reveal-plugins but that badly sucks.
Ideally npm install would be the easiest by far - I just pinged the other repo about that discussion; or, if not available, with a git subtree, if we can agree on that approach.

parmentelat · 2018-04-01T12:25:28Z

I just submitted a PR with the original chalkboard plugin

rajgoel/reveal.js-plugins#50

that aims at publishing the chalkboard plugin under npm, and I published a test version that is available here

https://www.npmjs.com/package/reveal.js-chalkboard

If the original author is willing to follow up on that, it would mean that I can completely review/simplify the way this PR installs the plugin

with the assumption that reveal.js-chalkboard is available at npmjs.com, and so without the need to duplicate the plugins code in this repo

parmentelat · 2018-04-01T17:38:05Z

I have rebased my branch so as to get rid of the addition of the plugin code

keyboard events management remains as the one issue that is left

damianavila · 2018-04-11T12:13:22Z

I have rebased my branch so as to get rid of the addition of the plugin code

Amazing! Thanks!!

keyboard events management remains as the one issue that is left

Do you want to work on that before I look into review this one?

parmentelat · 2018-04-11T12:24:35Z

keyboard events management remains as the one issue that is left

Do you want to work on that before I look into review this one?

well, ideally yes, but honestly my priorities about this one PR have dropped a little; so maybe it's best if you take a look at what we have right now already.

damianavila · 2018-04-11T13:07:00Z

so maybe it's best if you take a look at what we have right now already.

That's ok, I will try to review it soon. We can leave the keyboard thing as an enhancement for later, if it is necessary.

damianavila · 2018-05-21T16:47:17Z

package.json

-    "build-reveal": "cp -r ./node_modules/reveal.js rise/static/reveal.js",
-    "reset-reveal": "sed -i.upstream '11 s_^_\/*_' rise/static/reveal.js/css/reveal.css",
+    "build-reveal": "for dep in reveal.js reveal.js-chalkboard; do cp -r ./node_modules/$dep rise/static/$dep; done",
+    "reset-reveal": "sed -i.upstream '11 s_^_/*_' rise/static/reveal.js/css/reveal.css",


There is a missing backslash on this line, are you testing this with OSX, right?
I would prefer to keep the backslash because it is harmless for OSX and I have tested successfully on linux, in the past...

Eventually, we can use the reveal_rise package I created for the upcoming jupyterlab_rise, ref #381 (comment) to provide the proper CSS and get rid of this hack...

damianavila · 2018-05-21T16:51:19Z

rise/static/main.js

+                options.dependencies.push({ src: require.toUrl('./reveal.js/plugin/chalkboard/chalkboard.js'), async: true });
+                // xxx need to explore the option of registering jupyter actions 
+                // and have jupyter handle the keyboard entirely instead of this approach
+                // could hopefully avoid conflicting behaviours in case of overlaps


makes sense for later...

damianavila · 2018-05-21T16:52:59Z

rise/static/main.js

+                           110: function() { RevealChalkboard.toggleNotesCanvas() }, // 'N' toggle notes (slide-local)
+                           68:  function() { RevealChalkboard.download() },          // 'D' download recorded chalkboard drawing
+                           100: function() { RevealChalkboard.download() },          // 'd' download recorded chalkboard drawing
+                           // when 'd' is pressed


I do not understand this comment...

damianavila · 2018-05-21T16:55:06Z

rise/static/main.js

+              }
+
+
+              Reveal.initialize(options);


Can you remind me why you are using initializehere instead of configure?

This is a point that the original author had pointed out, so in his own words:

Reveal.configure does not load dependencies

damianavila · 2018-05-21T16:55:59Z

rise/static/main.js

@@ -663,32 +705,18 @@ define([
    var help_button = $('<i/>')
        .attr('id','help_b')
        .attr('title','Reveal Shortcuts Help')
-        .addClass('fa-question fa-4x fa')
+        .addClass('fa-question fa-3x fa')


Why are you changing the size here? Smaller things fit better with the chalkboard stuff?

IIRC, yes that's the spirit, there are new buttons related to chalkboard, so this looked better, but it's clearly a matter of taste, feel free to undo of course

damianavila · 2018-05-21T16:59:55Z

src/less/main.less

+
+/* use !important to override stuff that
+   would normally be configured through
+   chalkboard options */


And why you can't configure them using the chalkboard options? Because you use a set of config options by default and there is not an easy way to access the chalkboard config mechanism?

again it's been a long time so I'm not 100% positive here
IIRC since we load the chalkboard extension as-is, it comes with settings in its own css, but we want to override these, hence !important

damianavila · 2018-05-21T17:01:30Z

@parmentelat, I left some comments.
If you can also resolve the merge conflicts (merge or rebase on master), that would be great.

# Conflicts: # package.json # rise/static/main.js

parmentelat · 2018-05-22T17:27:49Z

merge has been done; see also my answers to your comments
generally speaking, this in my view is in standby mode for now
I'd like to urge you to release 5.3.0 before pursuing this one

damianavila · 2018-05-23T14:42:48Z

I'd like to urge you to release 5.3.0 before pursuing this one

That's the plan, but I felt I needed to provide you with feedback on this one after a long time without review from me.

…rd-redeux

damianavila · 2018-08-13T03:13:49Z

@parmentelat I did some updates and PR'ed against your branch: https://github.com/parmentelat/RISE/pull/1. If you can review my changes and test it, that would be awesome. Thanks!!

damianavila · 2018-08-16T14:08:26Z

Closing in favor of #408.
@parmentelat, thanks for the work you have done on this PR. I just submitted #408 with some fixes to be able to merge into master soon.

parmentelat changed the title ~~Chalkboard redeux~~ chalkboard redeux Mar 1, 2018

parmentelat mentioned this pull request Mar 2, 2018

Keep ? from popping up keyboard shortcuts? #354

Closed

This was referenced Mar 4, 2018

Possibility to configure reveal.js plugins #356

Open

making rise compliant with nbextensions_configurator #344

Merged

readme in configurator #357

Closed

This was referenced Mar 23, 2018

Added support for a chalkboard plugin #226

Closed

documentation review #347

Merged

parmentelat mentioned this pull request Mar 24, 2018

any plan to distribute this through npm ? rajgoel/reveal.js-plugins#46

Closed

rebased previous attempt

ad93b1b

with the assumption that reveal.js-chalkboard is available at npmjs.com, and so without the need to duplicate the plugins code in this repo

tweaks in the example notebooks to point at the new doc location

e918f31

damianavila reviewed May 21, 2018

View reviewed changes

Merge branch 'master' into chalkboard-redeux

5cb18e1

# Conflicts: # package.json # rise/static/main.js

Merge commit '7a89959834eae447df8f5119d55edc9bc3359590' into chalkboa…

b0c33b8

…rd-redeux

damianavila mentioned this pull request Aug 16, 2018

Chalk fixes on top of issue 355 #408

Merged

damianavila closed this Aug 16, 2018

parmentelat deleted the chalkboard-redeux branch June 26, 2019 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chalkboard redeux #355

chalkboard redeux #355

parmentelat commented Mar 1, 2018

damianavila commented Mar 1, 2018

parmentelat commented Mar 2, 2018 •

edited

Loading

damianavila commented Mar 8, 2018

parmentelat commented Mar 9, 2018

parmentelat commented Mar 9, 2018

damianavila commented Mar 9, 2018

parmentelat commented Mar 23, 2018

parmentelat commented Mar 24, 2018

parmentelat commented Apr 1, 2018

parmentelat commented Apr 1, 2018 •

edited

Loading

damianavila commented Apr 11, 2018

parmentelat commented Apr 11, 2018

damianavila commented Apr 11, 2018

damianavila May 21, 2018

damianavila May 21, 2018

damianavila May 21, 2018

damianavila May 21, 2018

damianavila May 21, 2018

parmentelat May 22, 2018

damianavila May 21, 2018

parmentelat May 22, 2018

damianavila May 21, 2018

parmentelat May 22, 2018

damianavila commented May 21, 2018

parmentelat commented May 22, 2018

damianavila commented May 23, 2018

damianavila commented Aug 13, 2018

damianavila commented Aug 16, 2018

chalkboard redeux #355

chalkboard redeux #355

Conversation

parmentelat commented Mar 1, 2018

damianavila commented Mar 1, 2018

parmentelat commented Mar 2, 2018 • edited Loading

damianavila commented Mar 8, 2018

parmentelat commented Mar 9, 2018

parmentelat commented Mar 9, 2018

damianavila commented Mar 9, 2018

parmentelat commented Mar 23, 2018

parmentelat commented Mar 24, 2018

parmentelat commented Apr 1, 2018

parmentelat commented Apr 1, 2018 • edited Loading

damianavila commented Apr 11, 2018

parmentelat commented Apr 11, 2018

damianavila commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented May 21, 2018

parmentelat commented May 22, 2018

damianavila commented May 23, 2018

damianavila commented Aug 13, 2018

damianavila commented Aug 16, 2018

parmentelat commented Mar 2, 2018 •

edited

Loading

parmentelat commented Apr 1, 2018 •

edited

Loading