Feature/oculus go tlc #467
Conversation
| <HomeRoot | ||
| initialEnvironment={qs.initial_environment} | ||
| dialogType={qs.list_signup ? InfoDialog.dialogTypes.updates : null} | ||
| />, |
There was a problem hiding this comment.
You're going to need to resolve a conflict with master here since qs became a URLSearchParams.
| <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> | ||
| <% } %> |
There was a problem hiding this comment.
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.
|
|
||
| if (props.initialEnvironment) { | ||
| environmentIndex = props.environments.findIndex( | ||
| e => e.name.toLowerCase() === props.initialEnvironment.toLowerCase() |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
IOW I think a case insensitive match here for the name of the environment is the proper UX for this parameter.
This PR closes out a few odds-and-ends:
productionmode for webpack dev server inyarn startinitial_environmentquery string parameter to the landing page, which we are going to need for the user testing in a few weeks