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.

Sign up for GitHub

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

Jump to bottom

Feature/oculus go tlc #467

Merged
merged 5 commits into from
Jul 20, 2018
Merged

Feature/oculus go tlc #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
437a326
Add aframe to link page due to new dependency
gfodor Jul 20, 2018
5001ccc
Skip entry if in VR already
gfodor Jul 20, 2018
206e34a
Add firefox reality test
gfodor Jul 20, 2018
33d37a2
Stop using production mode in yarn start
gfodor Jul 20, 2018
fd28ad8
Allow specification of initial environment
gfodor Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
},
"scripts": {
"postinstall": "node ./scripts/postinstall.js",
"start": "cross-env NODE_ENV=development webpack-dev-server --mode=production",
"start": "cross-env NODE_ENV=development webpack-dev-server",
"build": "rimraf ./public && cross-env NODE_ENV=production webpack --mode=production",
"doc": "node ./scripts/doc/build.js",
"prettier": "prettier --write '*.js' 'src/**/*.js'",
Expand Down
2 changes: 2 additions & 0 deletions src/hub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,8 @@ const onReady = async () => {
getAvailableVREntryTypes().then(availableVREntryTypes => {
if (availableVREntryTypes.gearvr === VR_DEVICE_AVAILABILITY.yes) {
remountUI({ availableVREntryTypes, forcedVREntryType: "gearvr" });
} else if (availableVREntryTypes.isInHMD) {
remountUI({ availableVREntryTypes, forcedVREntryType: "vr" });
} else {
remountUI({ availableVREntryTypes });
}
Expand Down
5 changes: 4 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ const qs = queryString.parse(location.search);
registerTelemetry();

ReactDOM.render(
<HomeRoot dialogType={qs.list_signup ? InfoDialog.dialogTypes.updates : null} />,
<HomeRoot
initialEnvironment={qs.initial_environment}
dialogType={qs.list_signup ? InfoDialog.dialogTypes.updates : null}
/>,
Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to need to resolve a conflict with master here since qs became a URLSearchParams.

document.getElementById("home-root")
);
6 changes: 6 additions & 0 deletions src/link.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
<title>Enter Code | Hubs by Mozilla</title>
<link href="https://fonts.googleapis.com/css?family=Zilla+Slab:300,300i,400,400i,700" rel="stylesheet">
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no">

<% if(NODE_ENV === "production") { %>
<script src="https://cdn.rawgit.com/aframevr/aframe/1be48d9/dist/aframe-master.min.js" integrity="sha384-SXrfoMHbXpA5RZhIyhgaR6tQ764dDZsbFk3PiokC/tc0+NnW1yaYQMUzWtL06hnq" crossorigin="anonymous"></script>
<% } else { %>
<script src="https://cdn.rawgit.com/aframevr/aframe/1be48d9/dist/aframe-master.js" integrity="sha384-AmjDGOMbvTrrUFdeVWcBIlXRINIWnO8iwj/4VS21OWbYDsa/7nheOIyPAPJSkR6J" crossorigin="anonymous"></script>
<% } %>
Copy link
Contributor

@mqp mqp Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why did this become necessary? Ah, I see you mentioned it was already necessary and this is just fixing breakage.

We really gotta get this into npm.

</head>

<body>
Expand Down
10 changes: 8 additions & 2 deletions src/react-components/home-root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ addLocaleData([...en]);
class HomeRoot extends Component {
static propTypes = {
intl: PropTypes.object,
dialogType: PropTypes.symbol
dialogType: PropTypes.symbol,
initialEnvironment: PropTypes.string
};

state = {
Expand Down Expand Up @@ -155,7 +156,12 @@ class HomeRoot extends Component {
</div>
</div>
<div className="hero-content__create">
{this.state.environments.length > 0 && <HubCreatePanel environments={this.state.environments} />}
{this.state.environments.length > 0 && (
<HubCreatePanel
initialEnvironment={this.props.initialEnvironment}
environments={this.state.environments}
/>
)}
</div>
</div>
<div className="footer-content">
Expand Down
13 changes: 11 additions & 2 deletions src/react-components/hub-create-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,25 @@ const HUB_NAME_PATTERN = "^[A-Za-z0-9-'\":!@#$%^&*(),.?~ ]{4,64}$";
class HubCreatePanel extends Component {
static propTypes = {
intl: PropTypes.object,
environments: PropTypes.array
environments: PropTypes.array,
initialEnvironment: PropTypes.string
};

constructor(props) {
super(props);

let environmentIndex = Math.floor(Math.random() * props.environments.length);

if (props.initialEnvironment) {
environmentIndex = props.environments.findIndex(
e => e.name.toLowerCase() === props.initialEnvironment.toLowerCase()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like code like this that tries to complicatedly fuzzy match what the inputs were supposed to mean, because I think it adds complexity with the result of making it harder to remember how the code works and more complicated to document. I wouldn't have done it and I would spend the time to actively work to undo it later by identifying and canonicalizing the actual parameters people use.

(This is a disagreement I have in other places in our code too, not just this one.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what your use case is for setting the initial environment, but I would consider making an error message pop out here if you got an invalid input (i.e. an environment that doesn't exist) instead of having a mysterious failure down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended for us to be able to tell the user study facilitators (perhaps verbally) how to initialize the landing page to a specific environment choice, and so I made it case insensitive (in part based upon the hilarious experiences of trying to get people to type "mediaTools=true" over the last couple of weeks and not getting the casing right)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IOW I think a case insensitive match here for the name of the environment is the proper UX for this parameter.

);
}

this.state = {
ready: false,
name: generateHubName(),
environmentIndex: Math.floor(Math.random() * props.environments.length),
environmentIndex,
// HACK: expand on small screens by default to ensure scene selection possible.
// Eventually this could/should be done via media queries.
expanded: window.innerWidth < 420
Expand Down
3 changes: 2 additions & 1 deletion src/utils/vr-caps-detect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export async function getAvailableVREntryTypes() {
const ua = navigator.userAgent;
const isSamsungBrowser = browser.name === "chrome" && /SamsungBrowser/.test(ua);
const isOculusBrowser = /Oculus/.test(ua);
const isInHMD = isOculusBrowser;
const isFirefoxReality = /Firefox Reality/.test(ua);
const isInHMD = isOculusBrowser || isFirefoxReality;

// This needs to be kept up-to-date with the latest browsers that can support VR and Hubs.
// Checking for navigator.getVRDisplays always passes b/c of polyfill.
Expand Down