-
Notifications
You must be signed in to change notification settings - Fork 169
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
Touch controls #87
base: main
Are you sure you want to change the base?
Touch controls #87
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/racing-game/CGa3wYGcYyCpovMmzojxEx3v2Jj4 [Deployment for 0c37f76 failed] |
beautiful! can we make the minimap a little smaller? i also think the on screen hints have to go, it's too little space for that. |
I ain't sure, but this is targeted for portrait mode? I think the game is better played on landscape mode then portrait mode as the car itself will already take half of your screen. But good begin! |
I actually would prefer to disable the minimap on mobile as well.
One option would be to move the camera back a little more on mobile. |
Yeah that seems good to me. |
right, lets remove the minimap, makes no sense on a small screen. |
src/styles.css
Outdated
} | ||
.mobile-controls .left-turn, | ||
.mobile-controls .right-turn { | ||
-webkit-user-select: none; |
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.
These are all redundancy, user-select
should cover all cases. It isn't like we should have to cover browsers from a decade ago(Luckily)
src/styles.css
Outdated
} | ||
|
||
@media (hover: hover) { |
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.
Hmm we are now going to rely into media queries to detect, touch devices? Well I have no experience with this specific media query and don't know the reliability of this. It should be hold in mind that it could break with edge cases.
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.
Thereby a detect if any keys are pressed(mobiles/tablets with keyboards) that we could add a non-touch
class into the html and then as well hide these.
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 suggest we use a package to handle this: https://github.com/duskload/react-device-detect
so we don't have to worry about the edge cases
src/controls/Mobile.js
Outdated
useEffect(() => { | ||
const downHandler = (e) => { | ||
if (target.indexOf(e.target.value) !== -1) { | ||
const isRepeating = !!pressed[e.target.value] |
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.
The value of this array is already a boolean
or null
no need to have the !!
in check as in the if
statement the browser will already check it for truthiness.
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.
The controls have changed since this PR has been opened
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.
Have all of the controls changed or specific controls that can get updated?
please list them.
How can we set this pr up for success? @njm222 , thanks
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.
From what I remember this is good to go, it just needs to be merged with main and resolve conflicts.
I would rebase onto main or force push a new branch since a lot has changed since this PR was opened.
Co-authored-by: Gusted <williamzijl7@hotmail.com>
is this ready to go? |
I don't think so |
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.
No more issues raised for me other then that little comment.
src/App.jsx
Outdated
<Speed /> | ||
<Help /> | ||
<KeyboardControls /> | ||
<HideMouse /> | ||
{isMobile && <MobileControls />} |
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.
{isMobile && <MobileControls />} | |
{isMobile ? <MobileControls /> : <KeyboardControls />} |
src/controls/Keyboard.js
Outdated
useKeys() | ||
if (isMobile) { | ||
useTouch() | ||
} |
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.
useKeys() | |
if (isMobile) { | |
useTouch() | |
} | |
if (isMobile) { | |
useTouch() | |
return | |
} | |
useKeys() |
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 would leave it like this so you can use both the touch and keyboard control on an iPad.
@motcodes is attempting to deploy a commit to the Poimandres Team on Vercel. A member of the Team first needs to authorize it. |
I added some simple touch-based controls:
I moved the speedometer and controls menu to the top when using a touch-enabled device.
And after clicking "click to continue" I unmount the start menu to prevent text selection.