Add view mode

[Hywan]

Ok, this patch adds the view mode. You can access to this mode by typing “Esc”. The patch simply adds class="view" to and CSS follows.

[pci]

Can I add I think this is brilliant!

I've added two very small suggestions in the code, but I can't wait for this to be in master :)

[Hywan]

Sorry for the late answer. I agree your comments. How should I proceed @paulrouget: should I propose a new patch?

[reagle]

Justing touching this issue as I'm still waiting for this myself in mainline.

[Hywan]

Ready!

[hsablonniere]

Man, your work is awesome!!! Can't believe I'm just seeing this after 3 months. I've tried it on my courses, it's brilliant.

Some remarks :

• You add a lot of event listeners, in setupView. Could you use event delegation and just add one?
• I think viewing incrementals completely would work better. Maybe with something like that : .view .incremental {visibility: visible; } at the end of default styles.
• You forgot -ms-transition: none; in .view section
• Embedder could have a icon button and dispatch the message via the protocol. http://www.utf8-chartable.de/unicode-utf8-table.pl
• If embedder can use this view mode, because of almost-here fullscreen feature, esc is not the best choice for this. A letter maybe?
• Onstage could use that to choose a slide instead of asking for cursor position.

Everyone, what are your opinion on my remarks?

I think we're looking at small adjustments for the merge. We'll handle the shells just after.

@Hywan Don't hesitate to merge your commits, we don't need code reviews iteration in history.

Thanks again.

[paulrouget]

What @hsablonniere said.

Feel free to file follow-up bugs and iterate on this (especially for the onstage integration)

[paulrouget]

(to merge the commits, use git rebase and squash: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)

[hsablonniere]

Note :

Embedder usage can be implemented for v2. On stage integration will be done after. See #60.

[paulrouget]

@Hywan Will you have some time soon to fix this? If not, let me know, I can take over your code. Thanks!

[Hywan]

Yup. I will do this today. Sorry for the late reply :-(.

[paulrouget]

Any update on that? Also, if ESC is pressed while in view mode, can we go back to the slide we were in?

[Hywan]

I'm working on. Will commit soon.

Any update on that? Also, if ESC is pressed while in view mode, can we go back to the slide we were in?

[hsablonniere]

I already said it but ESC is not the right choice for this keyboard shortcut since it's the default way to escape from fullscreen mode...

[paulrouget]

Good catch. What I meant is pressing the key (what ever the key is) should toggle the view mode (not just start it).

[codeincontext]

What key would be better, given that ESC is unsuitable? O, for overview?

Thanks for the updates on this. I love this feature.

[hsablonniere]

O feels right for me...

[Hywan]

“O” feels also right for me (in lowercase of course). Maybe we should add the ability to parameterize it?

[hsablonniere]

Humm I don't think we need a way to customize shortcuts...

[Hywan]

Why?

[codeincontext]

Well to be honest, the source is fairly open. Could just store it as var overviewShortcut = 'O' or equivalent.

[pci]

The main reason I can see for allowing it to be customized is if the user doesn't have a western keyboard, though I think most keyboard still have the Latin alphabet as well, given it's ubiquitousness on the internet.

However, the 'view' hotkey will only appear in one place in the code so shouldn't be too hard to change if you need to. The only advantage I can see is the "pull all static variable declarations out of code" rule some developers use to keep code tidy and readable.

Edit: If we did make it customizable, I'd suggest storing it in Dz.params.overviewShortcut

[paulrouget]

If you want to change it, you change the source code (like for all the other keys). That's it.

[hsablonniere]

I thought about it and maybe some variables could be moved to the same place for customizing purposes...

[Hywan]

Fine Paul :-). Patches are coming tomorrow.

[Hywan]

I have added some comments about my patch.

[Hywan]

Oops…

[hsablonniere]

There's still some stuffs to address :

• In setupView() : Event delegation, we just need to add one listener.
• In setupView() : sections already exist on the Dz object, no need to query the DOM again.
• In setupView() : html should be move to Dz.html like the slides it would allow you to reuse it in toggleView() without querying the DOM again.
• Your pull request still contains 3 commits. We only need one with Adding overview mode.

Don't hesitate if my remarks aren't clear enought or if you need help.

Thanks again for the great work...

[Hywan]

Important thing related to the last patch (that I have rebased and squashed): Dz.slides is now an Array instead of a NodeList (because I need to use theArray.indexOf() function); normally it does not impact the existing code (I have tested).

Is it ok for you guys?

[Hywan]

Grrr, I have got a lot of conflicts… If someone understands… Is it ok for you or not? I would like to propose one big patch (like I did with patch 82f698b) with my scrollIntoView fix (5048930), but I think I had some underlying conflicts (I'm new with Git, I used to use Mercurial actually). Need help I think.

[hsablonniere]

I did some git ninja magic and I rebased @Hywan commit onto master getting rid of all the strange commits ;-)

The commit author is really Hywan but I think the fact that I did the rebase myself prevents Github from linking the author correctly.

@paulrouget @Hywan What do you think?

If everything's OK I think we could finally offer this great feature to mainstream users and merge PR #86 and close this one.

[Hywan]

I was undecided about re-opening a pull request but you did it before me! Thanks. I'm ok with this. Go on :-).

[reagle]

I'm little confused on this one's status: is there anything holding this up from merging into master?

[reagle]

btw: In Google Chrome stable 18.0.1025.168-r134367 there's a slight line at the top slide/background boundary, see: http://reagle.org/joseph/talks/2012/test-overview.html#4.0 . (I don't see it in FF 12.0+build1-0ubuntu0.12.04.1 0).

[hoaproject]

@reagle Look at the PR #86.

[reagle]

On 05/03/2012 02:03 AM, Hoa wrote:

@reagle Look at the PR #58.

Unfortunately I have an remain confused. Could someone tell me when the overview and notes modes -- two distinct things -- are likely to be publicly released, that is appear here?

[hsablonniere]

About your bug on Chrome 18, I did a quick try but I'm not sure I follow you. Please file a separate issue with screenshots if needed.

[hsablonniere]

About the holding, it was a misunderstanding from @paulrouget and myself. Each one was waiting for the other. Sorry ;-)

[hsablonniere]

Overview mode just landed in master.

Notes mode (or maybe another name) is a different thing. I think we coverd the details in #32.

If you look at the labels. The only thing we miss for v2 is documentation. Work is in progress on that. Once it's finished you'll be able to download a dzslides-v2 archive (or sth like that).

RFE stands for Request For Enhancement. It's just the issues that represents future improvements/features. They will be assigned to a new milestone once v2 is ready.

Hope I answered all your questions ;-)

[hsablonniere]

View mode #86 just landed in master!!

[reagle]

Thank you. I confirm overview mode in master is working -- I can't replicate the line I saw last night in Chrome -- and I understand notes mode is still an open enhancement.

[hsablonniere]

Glad we could clarify stuffs for you.