#316 Fix cursor visibility on monitor fullscreen view #319
Conversation
|
Hi @cksharma11, thanks for submitting this PR! Unfortunately it isn't quite as simple this. Your change will always show the mouse cursor while in full screen. However the desired behaviour is for the mouse cursor to be hidden until the mouse is moved when in fullscreen. Which I'm now realising isn't quite as simple as I first thought. Adding a class when the mouse moves is simple, but removing it is a little more complicated (without duplicating the logic used by the fullscreen hook) 🤔 |
|
@GentlemanHal Ahh. Just realised that, will work on fixing the same. |
|
@GentlemanHal Updated the PR. Thought, I'm not sure if that's the correct way to do this? |
| const monitorClassNames = cn(styles.monitor, { | ||
| [styles.fullscreen]: fullScreen | ||
| }) | ||
| document.onmousemove = () => { |
There was a problem hiding this comment.
Using document.onmousemove is bad, it breaks React encapsulation meaning the handler isn't going to be cleaned up when the component unmounts. Which means a ton of warnings would be generated on every other page as you'd be trying to update state on an unmounted component!
| @@ -33,6 +33,11 @@ export function Monitor({fullScreen, requestFullScreen}: MonitorProps): ReactEle | |||
|
|
|||
| const [loaded, setLoaded] = useState(false) | |||
| const [projects, setProjects] = useState<Projects>([]) | |||
| const [monitorClassNames, setMonitorClassNames] = useState({ | |||
There was a problem hiding this comment.
You don't need to store all of this as state. You only really need to keep track of the inactive boolean. Also doing this means the fullscreen style will never get toggled, as the state is only initialised on the first render using the fullscreen boolean.
| }) | ||
| document.onmousemove = () => { | ||
| setMonitorClassNames({ | ||
| ...monitorClassNames, [styles.inactive]: false |
There was a problem hiding this comment.
Nothing is ever setting the inactive style back to true, so once the mouse is moved the mouse cursor will never be hidden again.
Thanks for updating the PR, but unfortunately not. I've added some comments directly to the code, as hopefully that's easier to explain some of the issues. |
|
I'm going to close this pull request, the code isn't mergeable and the underlying issue is more complicated than I realised. Sorry for wasting your time by incorrectly labelling the original issue 🙏 |
Fixes issue #316
@build-canaries/owners