-
Notifications
You must be signed in to change notification settings - Fork 172
Add a fullscreen button for the game div container #2855
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
Changes from all commits
177c86b
2031975
0832d8b
af140dc
d9e3a2e
31fc2c9
1a5b3a9
eef4ae8
f9e4c3e
5ffcabe
7ec8ad3
76e09cb
744b8b3
3c1a46d
4b50bab
daac858
e41fd96
a0d28d1
4f4718e
e2b6672
9c6b6bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
import { Icon } from '@blueprintjs/core'; | ||
import { IconNames } from '@blueprintjs/icons'; | ||
import { useFullscreen } from '@mantine/hooks'; | ||
import React from 'react'; | ||
import { useDispatch } from 'react-redux'; | ||
import { useTypedSelector } from 'src/commons/utils/Hooks'; | ||
|
@@ -50,9 +53,93 @@ function Game() { | |
} | ||
}, [session, achievements, goals]); | ||
|
||
// This is a custom hook imported from @mantine/hooks that handles the fullscreen logic | ||
// It returns a ref to attach to the element that should be fullscreened, | ||
// a function to toggle fullscreen and a boolean indicating whether the element is fullscreen | ||
const { | ||
ref: fullscreenRef, | ||
toggle: toggleFullscreen, | ||
fullscreen: isFullscreen | ||
} = useFullscreen<HTMLDivElement>(); | ||
|
||
// This function is a wrapper around toggleFullscreen that also locks the screen orientation | ||
// to landscape when entering fullscreen and unlocks it when exiting fullscreen | ||
const enhancedToggleFullscreen = async () => { | ||
toggleFullscreen(); | ||
|
||
if (window.screen.orientation) { | ||
if (!isFullscreen) { | ||
window.screen.orientation.lock('landscape'); | ||
} else { | ||
window.screen.orientation.unlock(); | ||
} | ||
} | ||
}; | ||
|
||
const gameDisplayRef = React.useRef<HTMLDivElement | null>(null); | ||
|
||
// This function sets the gameDisplayRef and also calls the ref callback from useFullscreen | ||
// to attach the fullscreen logic to the game display element | ||
const setGameDisplayRefs = React.useCallback( | ||
(node: HTMLDivElement | null) => { | ||
// Refs returned by useRef() | ||
gameDisplayRef.current = node; | ||
|
||
// Ref callback from useFullscreen | ||
fullscreenRef(node); | ||
}, | ||
[fullscreenRef] | ||
); | ||
|
||
// Logic for the fullscreen button to dynamically adjust its size, position and padding | ||
// based on the size of the game display. | ||
const [iconSize, setIconSize] = React.useState(0); | ||
const [iconLeft, setIconLeft] = React.useState('0px'); | ||
const [iconPadding, setIconPadding] = React.useState('0px'); | ||
|
||
React.useEffect(() => { | ||
const handleResize = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't put this function definition inside the |
||
if (gameDisplayRef.current) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the values never reset otherwise? |
||
const aspectRatio = 16 / 9; | ||
const height = gameDisplayRef.current.offsetHeight; | ||
const width = gameDisplayRef.current.offsetWidth; | ||
const size = height / 40; | ||
const padding = height / 50; | ||
const leftOffset = | ||
isFullscreen || height * aspectRatio > width ? 0 : (width - height * aspectRatio) / 2; | ||
setIconSize(size); | ||
setIconPadding(`${padding}px`); | ||
setIconLeft(`${leftOffset}px`); | ||
Comment on lines
+103
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get it:
|
||
} | ||
}; | ||
|
||
// When exiting fullscreen, the browser might not have completed the transition | ||
// at the time handleResize is called, so the height of gameDisplayRef.current | ||
// is still the fullscreen height. | ||
// To fix this, we delay handleResize by 100ms. | ||
const delayedHandleResize = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto on moving this out of the |
||
setTimeout(handleResize, 100); | ||
}; | ||
|
||
window.addEventListener('resize', delayedHandleResize); | ||
delayedHandleResize(); | ||
|
||
return () => window.removeEventListener('resize', delayedHandleResize); | ||
Comment on lines
+124
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the resize observer for when we already have |
||
}, [isFullscreen]); | ||
|
||
return ( | ||
<> | ||
<div id="game-display"></div> | ||
<div id="game-display" ref={setGameDisplayRefs}> | ||
<Icon | ||
id="fullscreen-button" | ||
icon={isFullscreen ? IconNames.MINIMIZE : IconNames.FULLSCREEN} | ||
color="white" | ||
htmlTitle={isFullscreen ? 'Exit full screen' : 'Full screen'} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, no particular reason. I was just looking for a way to implement a simple tooltip and came across using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any updates? I'll merge this PR first but please do change it in the future if you have the time (or create an issue if you don't). |
||
size={iconSize} | ||
onClick={enhancedToggleFullscreen} | ||
style={{ left: iconLeft, padding: iconPadding }} | ||
/> | ||
</div> | ||
{isTestStudent && ( | ||
<div className="Horizontal"> | ||
<button | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,9 @@ | |
width: 100%; | ||
align-items: center; | ||
} | ||
|
||
#fullscreen-button { | ||
position: absolute; | ||
cursor: pointer; | ||
z-index: 100; | ||
} |
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.
What is the purpose of this
gameDisplayRef
here, can we not just use the ref provided by theuseFullscreen
hook?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 don't think so. The
gameDisplayRef
is a React RefObject that I used in line 99 to 113 to handle the resizing of the game display when entering/exiting fullscreen. But the ref provided by theuseFullscreen
hook is a callback ref function. See https://mantine.dev/hooks/use-fullscreen/#definition.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.
My question is why do you need to handle the resizing manually? Doesn't
useFullscreen
already handle it for you?Uh oh!
There was an error while loading. Please reload this page.
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 added the fullscreen Icon element as a child of the game-display div element, which the Phaser game engine inserts the game canvas element in. The game canvas element has a fixed aspect ratio and its size is determined by the size of the game-display div element. When browser window width is too wide and height is too small, there will be empty space on the left and right of the game canvas element. So the left offset of the fullscreen Icon element is updated to ensure that it remains aligned with the top left of the game canvas element. The Icon size and padding were purely for aesthetic reasons.
I couldn't think of a better design since the game is a canvas element. To avoid the need for this, perhaps it would be better to implement the fullscreen option directly within the game itself. But I haven't gone into the Phaser game documentation yet so I'm not sure if it's possible.
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 the solution is very hacky. Why not just rely on the DOM to position the icon button correctly instead of using a resize observer? Instead of putting the button as a child you can just put it as a sibling, the wrap a parent container around both.
Maybe something like:
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.
Then you don't need to use any resize logic cause the DOM does the layouting for you
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 tried using
margin: auto
with the Icon button both as a child and as a sibling with a parent container wrapper. However, this causes issues with the resizing of the game canvas element since it references the game-display div element's width and height to determine its width and height. So, when you increase the size of the browser window, more margin gets added instead of the game canvas element getting enlarged.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 understand it now; I still feel the solution is hacky. After looking at game code, I have an idea for a refactor. Let me discuss with @lhw-1 first.