-
Notifications
You must be signed in to change notification settings - Fork 74
fix typo in fullscreen api #122
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
Conversation
the right spelling is using `screen` not `Screen` according to MDN: https://developer.mozilla.org/en-US/docs/Web/API/Document/fullscreenElement https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen https://developer.mozilla.org/en-US/docs/Web/API/Document/exitFullscreen
js/rpg_core/Graphics.js
Outdated
| Graphics._cancelFullScreen = function() { | ||
| if (document.cancelFullScreen) { | ||
| document.cancelFullScreen(); | ||
| if (document.fullscreenElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Could you fix this line to below?
if (document.exitFullscreen) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I changed it to:
if (document.exitFullscreen) {
document.exitFullscreen();
based on https://www.sitepoint.com/use-html5-full-screen-api
js/rpg_core/Graphics.js
Outdated
| */ | ||
| Graphics._isFullScreen = function() { | ||
| return ((document.fullScreenElement && document.fullScreenElement !== null) || | ||
| return ((document.fullscreenElement && document.fullscreenElement !== null) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method is very confusing.
line 1202 means Graphics._isFullScreen,
but line 1203, 1204 means actually Graphics._isNotFullScreen.
Could you fix it? 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I simplified it to
return document.fullscreenElement ||
document.mozFullScreen ||
document.webkitFullscreenElement ||
document.msFullscreenElement;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bungcip
I'm afraid this commit don't work.
Could you fix Graphics._switchFullScreen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch. after testing it with npm run watch and pressing F4 I can confirm this commit don't work. I will change it tonight.
bungcip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @krmbn0576, I already updated the code based on your comment.
js/rpg_core/Graphics.js
Outdated
| */ | ||
| Graphics._isFullScreen = function() { | ||
| return ((document.fullScreenElement && document.fullScreenElement !== null) || | ||
| return ((document.fullscreenElement && document.fullscreenElement !== null) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I simplified it to
return document.fullscreenElement ||
document.mozFullScreen ||
document.webkitFullscreenElement ||
document.msFullscreenElement;
|
new code in |
krmbn0576
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, LGTM! :-)
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
the right spelling is using
screennotScreenaccording to MDN:https://developer.mozilla.org/en-US/docs/Web/API/Document/fullscreenElement
https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen
https://developer.mozilla.org/en-US/docs/Web/API/Document/exitFullscreen