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

Feature/no slide looping #154

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

Conversation

barneyb
Copy link
Contributor

@barneyb barneyb commented May 20, 2016

if the new loop option is set false, don't loop around from last slide to first (or the reverse), and allow jumping directly to first/last slide using Home/End respectively

* Whether to allow looping from the last back to the first or backwards from
* the first to the last.
*/
var ALLOW_LOOPING = true;

/**
* Navigates forward n pages
Copy link
Owner

Choose a reason for hiding this comment

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

Was going to ask why not just use 1 for FIRST_SLIDE but, ack!, this method is weird. I like your solution :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, since it uses relative values, "1" means "advance one slide", so had to pick values outside any useful range. I suppose if someone ends up with a 10,000,000 slide presentation there will be problems with this feature, but it seems like that's a fair "bug" to leave latent.

@jdan
Copy link
Owner

jdan commented Jun 10, 2016

Heya @barneyb,

Thanks for this! Sorry for not reviewing sooner. I just made a couple small stylistic comments, but the code looks good to me. Thanks for the README changes too 👍

@jdan
Copy link
Owner

jdan commented Jun 11, 2016

Will merge later this afternoon when I'm at my computer, thanks again!

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

2 participants