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

Reveal 3.8.0 #475

Merged
merged 10 commits into from Jun 20, 2019
Merged

Reveal 3.8.0 #475

merged 10 commits into from Jun 20, 2019

Conversation

parmentelat
Copy link
Collaborator

this is WIP and not working; most obvious broken feature at this point is themes, and more specifically theme backgrounds, that do not render as expected

I'm exposing this so that others can experiment with and feedback about an angle that I think may be worth fiddling with

in a nutshell: as mentioned a few times in some other issues like #435 and #438, when running a RISE slideshow we primarily enable 3 source of css styling: jupyter, RISE and reveal

the way reveal themes are implemented for example sets attributes on body which tends to potentially have rather devastating effects

the angle I have taken in this one-line approach is to monkey-patch the official reveal themes (as downloaded after npm install) so as to make their body css rule more specific, and to give them a chance to override stuff - for example to override settings made in notebook.less on the body tag too.


this at this point is not working; it does improve the situation a bit, in that when inspecting the tag through web devel tools, the theme settings do show up as applying, but for some yet-to-be-figured reasons the background still gets rendered blank..

Any hint would be btw much appreciated about that matter, it currently beats me why background themes don't get rendered.

so that their settings on the <body> tag supersedes
stuff from notebook.less, that blindly puts a white background
@damianavila
Copy link
Owner

the angle I have taken in this one-line approach is to monkey-patch the official reveal themes (as downloaded after npm install) so as to make their body css rule more specific, and to give them a chance to override stuff - for example to override settings made in notebook.less on the body tag too.

I like that idea... even if it is not working at first... but I think it is a sensible path to investigate and push forward. I will try to run your branch soon to see if I can help with some more specific thoughts.

  (quicksand, which is the one that the 'sky' native theme comes with)
this change removes this choice, so we can more easily test themes
theme-patching code gets promoted to a plain helper script in patch-reveal/
@parmentelat
Copy link
Collaborator Author

hi Damian

rendering the themes properly turned out much harder than I anticipated, but I have eventually been able to do some progress on this topic

please give this a try when you get a chance

I'm rather pleased with the results in terms of theme rendering, although the approach is admittedly a little hacky; or not even a little ;) - I really don't see how to do otherwise, your feedback will be most welcome


FYI: if you are interested, I had initially gone even further in terms of theme integration
https://github.com/parmentelat/RISE/tree/more-theming-from-reveal

but that turned out too intrusive (code cells ended up way too small) so I've decided to keep that off the discussion for now at least; you may want to give that a try too if you're curious; but that again is just an FYI

@parmentelat parmentelat changed the title [WIP] Reveal 3.8.0 Reveal 3.8.0 Jun 17, 2019
@parmentelat
Copy link
Collaborator Author

I'm removing the [WIP] from this PR's title; as far as I am concerned this seems to be closing in

@damianavila
Copy link
Owner

rendering the themes properly turned out much harder than I anticipated,

I was expecting that, actually...

please give this a try when you get a chance

Sure!

I'm rather pleased with the results in terms of theme rendering, although the approach is admittedly a little hacky; or not even a little ;) - I really don't see how to do otherwise, your feedback will be most welcome

I love hacky! Jaja... I actually did some hacky things in the past so let's continue with the tradition 😉

FYI: if you are interested, I had initially gone even further in terms of theme integration
https://github.com/parmentelat/RISE/tree/more-theming-from-reveal
but that turned out too intrusive (code cells ended up way too small) so I've decided to keep that off the discussion for now at least; you may want to give that a try too if you're curious; but that again is just an FYI

OK, I will take a look!

@damianavila
Copy link
Owner

I will do a deep review as soon as I can, but looking around on the PR, it seems you are doing the same sort of changes with patch-themes and reset-reveal. Maybe it is now time to put those things together?

Another thing... we could be shipping a "new" npm package (as I did with the notes_rise npm package) built from that script you have written (maybe even include the notes_rise customizations in that package). If we do something like that, we abstract the hacky changes from RISE itself into an upstream package (that we control) which has all the customizations collected in just one place.

@parmentelat
Copy link
Collaborator Author

ok, I'm not sure that I follow completely but that sounds about right

from a completely different angle: I'd like to be able to build docker images with my version inside, so that I can test more thoroughly

I'm not quite sure how to do that; pip has the option to install from a git repo; but I'd be surprised if that worked as-is - I must admit that I have not even tried - because pip would not know to run npm install and the other build targets; or would it ?

is there any chance I/we can publish this dev version in a way that could be simply installed from a Dockerfile ?

@damianavila
Copy link
Owner

is there any chance I/we can publish this dev version in a way that could be simply installed from a Dockerfile ?

I would say let's push on this one, merge it and then we can release a 5.6.0 beta version. In fact, we should be releasing more betas as long as we merge potentially disruptive stuff...

@parmentelat parmentelat merged commit 60806a2 into damianavila:master Jun 20, 2019
@parmentelat
Copy link
Collaborator Author

taking up on your suggestion, I have merged this onto master and published 5.6.0.dev1 on pypi

try out with pip install --pre rise

@damianavila
Copy link
Owner

This is great @parmentelat!! I will test it soon.

Can we make a new issue to tackle some of the things I outlined in my quick review?

I will do a deep review as soon as I can, but looking around on the PR, it seems you are doing the same sort of changes with patch-themes and reset-reveal. Maybe it is now time to put those things together?

Another thing... we could be shipping a "new" npm package (as I did with the notes_rise npm package) built from that script you have written (maybe even include the notes_rise customizations in that package). If we do something like that, we abstract the hacky changes from RISE itself into an upstream package (that we control) which has all the customizations collected in just one place.

Additionally, we should probably release these dev versions with conda-forge as well...

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

2 participants