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

Fullscreen, remote control and overview navigation #711

Closed
wants to merge 0 commits into from

Conversation

giflw
Copy link
Contributor

@giflw giflw commented Dec 31, 2018

Add impress.js updated blackout and fullscreen plugins

This update add support for remote control presentation buttons:

  • blackout: using '.' key detection
  • fullscreen: enter with F5 and exit with ESC

Next and previous slides using PageUp/PageDown are already supported


This PR is being splitted into:

  • fullscreen
  • autoplay.js
  • blackout.js
  • help.js
  • navigation.js

@giflw
Copy link
Contributor Author

giflw commented Dec 31, 2018

Add autoplay pause on blackout and resume when removeBlackout

Add events to pause/resume autoplay

  • impress:autoplay:pause
  • impress:autoplay:resume
    Call those events from blackout

@giflw
Copy link
Contributor Author

giflw commented Dec 31, 2018

On firefox, after blackout, slides where partially rendered.

Copy link
Contributor

@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 @giflw for this new year's present! I have some minor comments and questions only.

src/plugins/fullscreen/fullscreen.js Outdated Show resolved Hide resolved
src/plugins/fullscreen/fullscreen.js Outdated Show resolved Hide resolved
src/plugins/blackout/blackout.js Outdated Show resolved Hide resolved
src/plugins/autoplay/autoplay.js Outdated Show resolved Hide resolved
src/plugins/blackout/blackout.js Outdated Show resolved Hide resolved
src/plugins/fullscreen/fullscreen.js Outdated Show resolved Hide resolved
@giflw giflw changed the title Add fullscreen and remote control presentation support Fullscreen, remote control and overview navigation Jan 1, 2019
Copy link
Contributor

@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.

Hi

Thanks for more work. The full help looks great, except does not conform to style / user experience. I will help you get it right.

But at this point it would be better to split this work into multiple small PRs. This is necessary to maintain a clean commit history where one thing is one commit. (You had done that originally, but then the fixes spoil it.) Also, some of your work is ready to be merged while other parts need changes still. This way we can merge the good parts already.

Could you create separate branch+PR for each:

  • fullscreen.js (this is ready to go I think)
  • autoplay.js (likewise)
  • blackout.js (please remove the impress:help:add for now)
  • help.js (Functionally you're doing the right thing, but this will need several changes.)
  • navigation.js (Needs to be done differently, we can discuss in new PR.)

Sorry for the extra work, but on the positive side, this will get most of your code merged asap rather than always doing a little bit more.

@giflw
Copy link
Contributor Author

giflw commented Jan 10, 2019

No problem. I'll do that as soon as I can.

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Added PR #712 for fullscreen

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Added PR #713 for pause/resume events on autoplay

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Changes on blackout needs autoplay to be merged, as it is used to pause/resume autoplay when enter/exit blackout

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Added PR #714 for full help screen on help plugin

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Added PR #716 for blackout support for . key

@giflw
Copy link
Contributor Author

giflw commented Jan 15, 2019

@henrikingo Added PR #717 for navigation (o toogle overview)

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