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.

Sign up for GitHub

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

added option to hide cursor in fullscreen mode #536

Closed
wants to merge 1 commit into from
Closed

added option to hide cursor in fullscreen mode #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
right around the corner of this tag ;)

-->
<div id="impress">
<div id="impress" data-hide-mouse-after="3000">
Copy link
Member

Choose a reason for hiding this comment

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

Hi sclausen

Good idea and easy to implement.

IMO this feature needn't be optional. (Not adding an option benefits the goal of simplicity.) I don't think anyone will miss the pointer if we just hide it always after 3 seconds of inactivity. (I can see that someone would want to leave the mouse pointer pointing at something on a slide. But that should be rare. You'd usually move the pointer to point at something, but that would still work. Also, it is more common to use laser pointers, etc.)


<!--

Expand Down
27 changes: 27 additions & 0 deletions js/impress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,33 @@
scale: 1
};

// Hide the mouse cursor in fullscreenmode after the configured
Copy link
Member

Choose a reason for hiding this comment

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

We are moving to a plugin architecture. So instead of a patch against impress.js, you should write this as a plugin. See https://github.com/henrikingo/impress.js/blob/myfork/src/plugins/README.md for a detailed explanation. If you want to write a PR against that repo, you're welcome to do so.

Alternatively, if you want to do a PR against the impress/impress.js repo in its current state, you could still write this as a "plugin" By adding a separate (function(window, document){...})(window, document); at the end of the file. You can take a queue from the keypress navigation at the end of the file, which is in fact such a plugin (even if it's not in a separate file).

// amount of milliseconds without movement
if(rootData.hideMouseAfter) {
var mouseMoveTimer = null,
isCursorVisible = true;

var hideCursor = function() {
//isFullscreen
if(!window.screenTop && !window.screenY){
Copy link
Member

Choose a reason for hiding this comment

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

This if() is actually a bit obscure. It would be more readable if you implement a helper function isFullscreen(), which returns true when this if is true. (I can get it since there is the comment, but would prefer the helper function, especially as this code moves to a separate plugin, so an extra line of code or two isn't an issue.)

mouseMoveTimer = null;
Copy link
Member

@henrikingo henrikingo Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm unsure about this line. If someone were to call hideCursor() some other way than via setTimeout() timeout triggering, then mouseMoveTimer will be null but the timeout will still expire? Wouldn't it be better to call window.clearTimeout(mouseMoveTimer) here?

Alternatively, I'm not sure you need to do anything about mouseMoveTimer here. Wouldn't this work just as well if you just leave this line out?

document.body.style.cursor = "none";
isCursorVisible = false;
}
};

document.onmousemove = function() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you should use document.addEventListener() instead. Other than that I agree with this approach.

if (mouseMoveTimer) {
window.clearTimeout(mouseMoveTimer);
}
if (!isCursorVisible) {
document.body.style.cursor = "default";
Copy link
Member

Choose a reason for hiding this comment

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

If the user wanted to theme the cursor with css, you will now unset that. You need to save the original cursor value when hiding it, then set it back to original value here.

isCursorVisible = true;
}
mouseMoveTimer = window.setTimeout(hideCursor, rootData.hideMouseAfter);
};
}

initialized = true;

triggerEvent(root, "impress:init", { api: roots[ "impress-root-" + rootId ] });
Expand Down