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

Change default interaction policy to always-allow #621

Merged
merged 3 commits into from Jul 17, 2019

Conversation

pushmatrix
Copy link
Contributor

@pushmatrix pushmatrix commented Jun 11, 2019

From all of our user testing it seems clear that requiring focus on mobile to interact is not intuitive. @mrdoob said the same thing over at #479.

I do think we need a better heuristic for mobile (#479), and we should have the interaction policy be configurable (#500), but a sensible default should favor making it easy to interact with.

Thoughts?

@elalish elalish requested a review from cdata June 19, 2019 16:27
@cdata cdata added this to the v0.6.0 milestone Jun 19, 2019
Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

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

This looks good. I think I would like to see this paired with a way for folks to opt-in to requiring focus until such time as we implement #479 . LMK if you have time to add an attribute for that, otherwise I can layer it on!

@pushmatrix
Copy link
Contributor Author

I can likely handle it, but it will be next week.

@pushmatrix
Copy link
Contributor Author

@cdata I added in the attribute interaction-policy so users can opt-in to requiring focus. This name came from the internal attribute that is currently being used.

  • Is interaction-policy a good name for an attribute? Or should it be something like require-focus and have that just be a true/false?

  • SmoothControls.ts and Controls.ts now both define the same InteractionPolicy. Doesn't seem DRY to me. Thoughts?

@cdata
Copy link
Contributor

cdata commented Jul 10, 2019

interaction-policy is a good name. I think attributes with explicit values are better than boolean attributes without values in a lot of cases (for the same reason that boolean method parameters are usually not ideal).

Agreed, there is a bit of repetition here. I think in the long-run, interaction-policy will be evolved or deprecated as we land on some solid built-in heuristics for interaction detection e.g. #479

Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

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

One small change requested, otherwise looks great 🍻

src/features/controls.ts Outdated Show resolved Hide resolved
src/features/controls.ts Show resolved Hide resolved
src/three-components/SmoothControls.ts Show resolved Hide resolved
index.html Show resolved Hide resolved
@pushmatrix
Copy link
Contributor Author

Changes made :)

Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 👍

@cdata cdata merged commit e3c03af into google:master Jul 17, 2019
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

2 participants