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

merge reveal templates #4795

Merged
merged 2 commits into from Jan 16, 2014
Merged

merge reveal templates #4795

merged 2 commits into from Jan 16, 2014

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 13, 2014

there was very fine-grained subclassing in reveal_internals in four template files, which I don't think was beneficial.

Ping @damianavila for review, in case this separation was used in ways I am not aware of.

there was very fine-grained subclassing in reveal_internals,
which I don't think was beneficial.
@damianavila
Copy link
Member

I will test it in depth tomorrow morning (late here now)... I think it would not be any problem...
The separation of templates were done following the design of the slides, but you are right about the problems to have it splitted... so I have no problem to make them into one...
I will give you a more detailed feedback in a couple of hours ;-)

{% from 'mathjax.tpl' import mathjax %}

{%- block any_cell scoped -%}
{%- if cell.metadata.slide_type in ['-'] -%}
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see this "do nothing cell" ['-'] after the subslides (I mean after the 13th line of the code)... I know that the current implementation is totally valid... but I think that putting it after the subslides is a conceptual enhancement because the new developer will have an immediate idea of the overall structure: slide>subslides and then intraslides properties such as: "-", skip, notes and fragments.

@damianavila
Copy link
Member

I just did one comment with a minor change I think is important... otherwise it is OK for me... (I also tested and it did not present any problem...)

@minrk
Copy link
Member Author

minrk commented Jan 15, 2014

reordered, thanks @damianavila

@damianavila
Copy link
Member

Thanks to you for the reordering...
For me, it is ready to merge 👍

Carreau added a commit that referenced this pull request Jan 16, 2014
@Carreau Carreau merged commit 811d665 into ipython:master Jan 16, 2014
@minrk minrk deleted the single-reveal-template branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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