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

Presenter View #43

Merged
merged 4 commits into from
May 17, 2011
Merged

Presenter View #43

merged 4 commits into from
May 17, 2011

Conversation

BestFriendChris
Copy link
Contributor

Hi there,

This isn't fully baked but I'd like to get your input on what I've already done.

Essentially, I wanted a second browser window containing a Presenter View. Currently, what this means is it shows the current slide, the next slide, and any presenter notes that you have added. Presenter notes can be added to a slide by added a header with the title "Presenter Notes"; Any content after that will be hidden from the slide and only showed in the Presenter View.

To see what I've currently done, load up a presentation and hit the '4' key. A second window should open up. Both windows should always point to the same slide (whether by moving left/right or using the TOC)

Thoughts?
Chris

…her window containing the current and next slide. Changing slides on either window will update the other.
…as follows:

    Presenter Notes 
    ===============
    Everything here will be hidden from the normal slide and shown in the Presenter View
@adamzap
Copy link
Owner

adamzap commented Apr 23, 2011

This is great!!

Once you're done, I'll merge it and cut 1.0. Great work, seriously.

@adamzap
Copy link
Owner

adamzap commented Apr 23, 2011

Or did you want someone else to finish it?

@BestFriendChris
Copy link
Contributor Author

Nope. I'm fine doing it. The only thing is my CSS skills suck, so I could probably use some help getting the presenter view to look nice, especially on other browsers.

One thing. What would be a good shortcut key for it? I did '4' mostly because I couldn't be bothered to look up keycodes. :-)

@adamzap
Copy link
Owner

adamzap commented Apr 24, 2011

Cool. I'll take a look at the CSS once you're done. No worries. Not that I'm a pro either though... ;)

'p' would be my first idea for a shortcut key. It's keycode 80.

@BestFriendChris
Copy link
Contributor Author

Here you go. It's not the prettiest bell at the ball, but it seems to work well.

One odd problem I ran into is on Chrome (on my mac) it is automatically sending the keydown event to the new window. I wanted 'p' to toggle the presenter view in both windows, but with this bug it closes immediately after opening. Not sure what's happening. For now I just ignore the event from the presenter view, so you have to close it manually. Not a big deal, but I thought you might have some ideas.

@adamzap
Copy link
Owner

adamzap commented Apr 30, 2011

Great, I'm about to merge. One question though.

Do you intend for markdown content (lists, for instance) to be rendered to presenter notes? I am considering replacing your heading notation with our typical macro or note notation:

.presenter-notes Blah Blah Blah Blah

Or something like that. To me, adding a header may confuse things a little. I'm still thinking about it.

Do you feel strongly either way?

I didn't encounter that bug in Chrome. I'll look deeper in a bit.

@BestFriendChris
Copy link
Contributor Author

Yeah, that was my intention. At work we tend to have a lot of content in the presenter notes. Having the entirety of markdown (for lists, headers, code, etc) is way useful. That being said, we definitely don't need it to be a header. Anything we can use as a marker to differentiate between the slide and the notes section would work.

@@ -255,6 +255,8 @@ class Generator(object):
"""
find = re.search(r'(<h(\d+?).*?>(.+?)</h\d>)\s?(.+)?', slide_src,
re.DOTALL | re.UNICODE)
presenter_notes = None
Copy link
Contributor

Choose a reason for hiding this comment

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

This should really be handled within a macro imho (but great work overall though)

@n1k0
Copy link
Contributor

n1k0 commented May 1, 2011

As I just said in the inline code comment above, I really think the whole presenter stuff should be handled by a dedicated Macro. I think the Generator class is bloated enough for now ;)

If you agree, I can help with refactoring the code if needed.

Awesome work and great feature anyway, can't wait to see it merged =)

@BestFriendChris
Copy link
Contributor Author

The only reason I didn't go the Macro route was because it seemed wrong to include the presenter notes within the 'inner' content div (much like how the footer exists outside of that div as well). Maybe a more generic solution would be to have the Macro (or a different type of Macro) support taking & returning the slide_vars directly. That way it could return a new 'presenter_notes' var and remove it from the 'content' var.

If you don't think it's a big deal no include it in the 'inner' div, let me know. I could probably be convinced of that fairly easily.

Thoughts?

@adamzap
Copy link
Owner

adamzap commented May 2, 2011

Yes, I'd like to move this to a macro as well. I was looking at that the other night. I'll let you know about the refactoring, n1k0. I might have some time early this week.

I'm mostly concerned with what the input format will look like. Right now I'm leaning toward something like this:

## Slide 1

Hello

Blah Blah

.presenter_notes

- One
- Two
- *Three*

## Slide Two

Thoughts?

@BestFriendChris
Copy link
Contributor Author

Looks good to me.

@n1k0
Copy link
Contributor

n1k0 commented May 5, 2011

I would personaly keep the trailing colon to keep a bit of consistency, eg. .presenter_notes:

@adamzap
Copy link
Owner

adamzap commented May 6, 2011

Oh yes. I forgot the trailing colon ;)

I'll do it that way. Thanks for the feedback, guys.

adamzap added a commit that referenced this pull request May 17, 2011
@adamzap adamzap merged commit cd19a18 into adamzap:master May 17, 2011
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