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.

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Touch controls #87

wants to merge 25 commits into from

Conversation

motcodes
Copy link

@motcodes motcodes commented Jun 8, 2021

I added some simple touch-based controls:

  • Forward
  • Backward
  • Left
  • Right
  • Boost
  • Reset

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.

@vercel
Copy link

vercel bot commented Jun 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/racing-game/CGa3wYGcYyCpovMmzojxEx3v2Jj4
✅ Preview: https://racing-game-git-fork-motcodes-touch-controls-pmndrs.vercel.app

[Deployment for 0c37f76 failed]

@drcmda
Copy link
Member

drcmda commented Jun 8, 2021

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.

@Gusted
Copy link
Contributor

Gusted commented Jun 8, 2021

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!

@motcodes
Copy link
Author

motcodes commented Jun 8, 2021

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 actually would prefer to disable the minimap on mobile as well.

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!

One option would be to move the camera back a little more on mobile.

@motcodes motcodes mentioned this pull request Jun 8, 2021
@Gusted
Copy link
Contributor

Gusted commented Jun 8, 2021

One option would be to move the camera back a little more on mobile.

Yeah that seems good to me.

@drcmda
Copy link
Member

drcmda commented Jun 8, 2021

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;
Copy link
Contributor

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/ui/Help.jsx Outdated Show resolved Hide resolved
src/ui/MobileControls.jsx Outdated Show resolved Hide resolved
src/styles.css Outdated
}

@media (hover: hover) {
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

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

useEffect(() => {
const downHandler = (e) => {
if (target.indexOf(e.target.value) !== -1) {
const isRepeating = !!pressed[e.target.value]
Copy link
Contributor

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.

Copy link
Member

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

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

Copy link
Member

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.

motcodes and others added 2 commits June 8, 2021 23:12
Co-authored-by: Gusted <williamzijl7@hotmail.com>
@drcmda
Copy link
Member

drcmda commented Jun 9, 2021

is this ready to go?

@njm222
Copy link
Member

njm222 commented Jun 9, 2021

I don't think so

@njm222 njm222 marked this pull request as draft June 9, 2021 07:45
@motcodes motcodes marked this pull request as ready for review June 9, 2021 11:46
src/controls/Mobile.js Outdated Show resolved Hide resolved
src/lib/useOrientationChange.js Outdated Show resolved Hide resolved
src/lib/useOrientationChange.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Gusted Gusted left a 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/lib/useOrientationChange.js Outdated Show resolved Hide resolved
src/App.jsx Outdated
<Speed />
<Help />
<KeyboardControls />
<HideMouse />
{isMobile && <MobileControls />}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{isMobile && <MobileControls />}
{isMobile ? <MobileControls /> : <KeyboardControls />}

Comment on lines 31 to 34
useKeys()
if (isMobile) {
useTouch()
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
useKeys()
if (isMobile) {
useTouch()
}
if (isMobile) {
useTouch()
return
}
useKeys()

Copy link
Author

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.

@vercel
Copy link

vercel bot commented Jun 10, 2021

@motcodes is attempting to deploy a commit to the Poimandres Team on Vercel.

A member of the Team first needs to authorize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants