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

css loading, header and footer: a proposal #296

Merged
merged 3 commits into from
Nov 3, 2017
Merged

css loading, header and footer: a proposal #296

merged 3 commits into from
Nov 3, 2017

Conversation

parmentelat
Copy link
Collaborator

This commit adds a couple features and related examples

  • css-loading (inspired by PR Added companion CSS file #200, a little extended), in that order

  • header and footer

    • looks for config tag 'overlay' - if found, this is used
      to build the constant background;
      it is wrapped in a

      , so can be text or html
      in this case user is entirely responsible for styling

    • otherwise looks for config tags 'header' and 'footer'
      in this case minimum styling is applied (floor and ceiling)
      but user is still responsible for cosmetic styling

See the examples directory for usages
I've taken that chance to add an example named showflow
to demonstrate other features related to controlling
the slideshow with space and shift-enter
this is unrelated but a harmless addition imho

Relates to:

#173 for header and footer
#200 for adding css loading

* css-loading (inspired by PR #200, a little extended), in that order

  * ./rise.css (so this will apply to all notebooks in the current dir)
  * ./<notebookname>.css - this namely is what PR #200 was about

* header and footer

  * looks for config tag 'overlay' - if found, this is used
    to build the constant background;
    it is wrapped in a <div>, so can be text or html
    in this case user is entirely responsible for styling

  * otherwise looks for config tags 'header' and 'footer'
    in this case minimum styling is applied (floor and ceiling)
    but user is still responsible for cosmetic styling

See the examples directory for usages
I've taken that chance to add an example named showflow
to demonstrate other features related to controlling
the slideshow with space and shift-enter
this is unrelated but a harmless addition imho

Relates to:

#173 for header and footer
#200 for adding css loading
@damianavila
Copy link
Owner

Wonderful @parmentelat, I would love to have more time to give you feedback on these PR more quickly. Hopefully I can review, test and merge several of them at the weekend. Sorry for the delay and thank you very much for your multiple contributions!

@parmentelat
Copy link
Collaborator Author

parmentelat commented Sep 21, 2017 via email

let overlay = config.get_sync('overlay');
let header = config.get_sync('header');
let footer = config.get_sync('footer');
let backimage = config.get_sync('backimage');
Copy link
Owner

Choose a reason for hiding this comment

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

I think we need to put default values here https://github.com/parmentelat/RISE/blob/master/rise/static/main.js#L25, for these options. Don't you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, I'm not quite sure now, but I think at the time I considered it but rules it out because of the config logic; IIRC having overlay defined kind of hides the ability to use header/footer/backimge (or it it the other way around?)

So probably having them in a comment could be helpful, but more than that I don't know

Copy link
Owner

Choose a reason for hiding this comment

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

OK, a comment sound like a good compromise.

// Attempt to load CSS with the same path as the .ipynb but with .css extension instead
let notebook_css = window.location.pathname.replace(/\.ipynb$/,'.css');
$('head').append(`<link rel="stylesheet" href="${notebook_css}" id="notebook-custom-css" />`);

Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense, you have a rise.css that could be more general applied first and then the notebook specific one applied on top of that...

@damianavila
Copy link
Owner

Some things:

  • We need to document how we can use these, I know the examples are good to know how it works, but we need, at least, to put some lines in the config section in the docs as well.

  • Are we allowed to use that background image? I don't not know the copyright for that image. If we don't have it clear, we need to use another public image.

@parmentelat
Copy link
Collaborator Author

  • generally speaking, it would make sense to rethink the doc entirely IMHO
    so I had some notes on how to go about that, but honestly would appreciate if all the code could settle at first

  • about the image;l that's the background I was playing with at the time for our video recording; both logos are public, so I would tend to think yes; however I cannot produce a formal agreement from both institutions, so if you want to play it safe please replace with any other image that you'd like

@parmentelat
Copy link
Collaborator Author

I need to go for today; let me know if you need anything else on my end for merging this

@damianavila
Copy link
Owner

it would make sense to rethink the doc entirely IMHO
so I had some notes on how to go about that

Great, would love to hear those thoughts.

but honestly would appreciate if all the code could settle at first

That's ok. We can do that 😉

if you want to play it safe please replace with any other image that you'd like

OK, I will probably do that to play safe.

@damianavila
Copy link
Owner

I need to go for today; let me know if you need anything else on my end for merging this

I think we are OK, have a fun rest of your day.

@damianavila damianavila removed the check label Nov 3, 2017
@damianavila damianavila merged commit 8125c68 into damianavila:master Nov 3, 2017
@damianavila damianavila mentioned this pull request Nov 3, 2017
@damianavila
Copy link
Owner

Adding some more minor pieces in #310.

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