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

Add roam plugin. #781

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add roam plugin. #781

wants to merge 8 commits into from

Conversation

kdxcxs
Copy link
Contributor

@kdxcxs kdxcxs commented Feb 20, 2021

Add roam plugin to allow the user to change the view freely.

Copy link
Member

@henrikingo henrikingo left a comment

Choose a reason for hiding this comment

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

Thanks kdxcxs!

This is very impressive. I have implemented something similar in impressionist however, unlike you I did miss CS computer graphics classes and you can tell the difference. I was impressed to read your clean implementation.

I have a small number of impress.js specific changes requested.

In addition, it could be nice to advertise this feature in some of the demo presentations. The classic-slides demo is intended to showcase most features, so you could add text somewhere - maybe to the last overview step - to advertise roam plugin. In addition it seems the 3D-rotations demo is kind of topical here?

src/plugins/roam/roam.js Outdated Show resolved Hide resolved
src/plugins/roam/roam.js Outdated Show resolved Hide resolved
src/plugins/roam/roam.js Show resolved Hide resolved
@kdxcxs
Copy link
Contributor Author

kdxcxs commented Mar 1, 2021

Thanks henrik. I'm really happy you like the idea and it's an honour to contribute to such a cool project!

Actually I'm not sure if it's better to move just the view instead of the active step.

Also, I couldn't come up with a impressive way to advertise it for now as I want to keep the demo cool and clean. I'll do my best.

@henrikingo
Copy link
Member

Actually I'm not sure if it's better to move just the view instead of the active step.

Of course. I just meant the overview step could have the text somewhere. Or it can just be a regular slide.

Also, I couldn't come up with a impressive way to advertise it for now as I want to keep the demo cool and clean. I'll do my best.

I can do it separately if you don't feel like it.

impressActive = false;
target.apply( thisArg, argumentsList );
}
} );
Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry. I should have pointed to this in my last review, but I had forgotten how exactly to do this. Please use

gc.pushCallback(my_function);

var pushCallback = function( callback ) {

... to register a function that is called as part of impress().tear().

Example:

gc.pushCallback( function() {

@henrikingo
Copy link
Member

Oh, and please run npm run all and then include js/impress.js in your commit. Due to backward compatibility impress.js has the built file directly in the git repo.

@henrikingo
Copy link
Member

Hi, again... So I downloaded your branch and tried it a bit. It works and is really cool, but...

I think this is what you referred to as well: It probably makes more sense not to move the current step and rather move the "view". Impress calls it the canvas element, in Impressionist I call it "camera". Anyway, this is how you get to the canvas element:
https://github.com/henrikingo/impressionist/blob/788e6d05e50287ea52911ae485a27054a46c3516/src/plugins/camera/camera.js#L82

It's a <div> inserted dynamically between the div#impress element and the .step elements. Moving the canvas can be done with exact same functions as you have, except I believe you have to move in the opposite direction compared to what you are doing now.

Moving the canvas means that I can "roam" around the entire presentation, but the slides don't move in relation to each other.

Minor: For the left hand, I wonder if it would be more natural to use keys a-f, w-s, e-d instead? (Corresponding to left-right, up-down, in-out, respectively.) The way you have it now, I would expect w-s to move up-down. But this is subjective, it's equally correct the way you've implemented it.)

@henrikingo henrikingo mentioned this pull request Oct 31, 2023
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