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

feat: add support for click element selector to PointerLockControls #285

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

jure
Copy link
Contributor

@jure jure commented Feb 3, 2021

Why

When several elements that "activate" the experience are present on the screen (e.g. a 'Enter VR button', a 'Click here to play' button, etc.), it conflicts with PointerLockControl's top-level document.addEventListener element binding.

What

This PR adds support for binding the control activation to an element found with a selector (e.g. #instructions). The default of document is unchanged, so this is not a breaking change. I also removed the target prop as it has no effect for PointerLockControls (while it's present for most controls, it's only applicable for OrbitControls and TrackballControls - this is addressed in #286)

Checklist

  • Documentation updated
  • Storybook entry added
  • Ready to be merged

Not yet ready to merge as there are a few things to discuss, however.

1. Usually all props are spread over into the primitive, but in this case the selector prop is only used on the drei side. I've introduced an internalProps variable, that removes the drei-only props before passing them onwards, but I'm wondering if there's an existing general principle I should uphold here. Update: I used the prop spread method from TransformControls to solve this.
2. How should a story for this be written? A realistic example depends on an HTML element that generally lives outside of the canvas, but the stories all live within the canvas. Update: Done following Josh’s feedback.

@vercel
Copy link

vercel bot commented Feb 3, 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/drei/9y1aqusyn
✅ Preview: https://drei-git-fork-jure-pointerlockcontrolsselector.pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 3, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 48da7af:

Sandbox Source
bitter-sunset-od77b Configuration
drei reflector Configuration

@joshuaellis
Copy link
Member

joshuaellis commented Feb 3, 2021

I'm gonna move this to a draft until we're ready for merge.

the stories all live within the canvas.

That's because the root story is passed to "set up" you could bypass and render a complete scene in the story:

export default {
  title: 'Controls/PointerLockControls',
  component: PointerLockControlsScene,
 /*
  * you could remove this decorator and move it into a component 
  * that renders the external HTML you're talking about.
  */
  decorators: [(storyFn) => <Setup cameraPosition={[0, 0, 10]} controls={false}>{storyFn()}</Setup>],
}

@jure
Copy link
Contributor Author

jure commented Feb 3, 2021

Added the story - this is ready for review now from my side.

@jure jure marked this pull request as ready for review February 3, 2021 14:26
@joshuaellis joshuaellis changed the base branch from master to beta February 3, 2021 18:21
@joshuaellis joshuaellis merged commit ad42f1f into pmndrs:beta Feb 3, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

🎉 This PR is included in version 3.3.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released on @beta released in beta for review label Feb 3, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants