Add HTML history + back button to entry flow #823
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
netpro2k
reviewed
Jan 7, 2019
There was a problem hiding this comment.
Glad we are getting a router setup, as we add more stuff it will probably save us some mess. I also really like being able to go back in the entry flow, as I have had to reload tat due to a miss-click many times.
I do think we should simplify the back button rules to just:
- if you are at the start of the entry flow, back returns to wherever you came to hubs from
- If you are in the entry flow, back returns to previous step of the entry flow
- if you are in a modal, back closes the modal
- if you are in the room, back returns to the entry flow
I don't much see the point of hitting back to re-enter a modal, and feel like it will only build up a confusing habit that sometimes ends up booting you from the room on accident.
Also, made some suggestions on cleaning up the two hacks you had in the entry flow.
src/react-components/ui-root.js
Outdated
| // | ||
| // This makes it so if we are in the room and hit back in the browser, we go to the URL | ||
| // the browser was on before the entry flow, not back into the entry flow. | ||
| this.props.history.listen((newLocation, action) => { |
There was a problem hiding this comment.
Hmm this seems super fishy. The entered state should just be a route, and that route should replace all the entry flow routes in the history stack.
src/react-components/ui-root.js
Outdated
| // | ||
| // Note this isn't perfect, if we refresh the page mid-entry flow and then hit back, we end | ||
| // up in a bad state unless we were on the first step. But this seems reasonable enough for now. | ||
| if (ENTRY_FLOW_ROUTES.find(x => x === this.props.history.location.pathname)) { |
There was a problem hiding this comment.
Not 100% sure but I think you can push states with the same path, so instead of making each entry flow step its own path we can just have a pushState that has the current flow step in it, that way back button still works, but the url doesn't change so refreshing still goes back to the beginning for the flow. This combined with a replace on the actual entry should fix these 2 hacks.
src/hub.js
Outdated
| const Router = | ||
| process.env.RETICULUM_SERVER && process.env.RETICULUM_SERVER !== document.location.host | ||
| ? HashRouter | ||
| : BrowserRouter; |
There was a problem hiding this comment.
I remember some nightmares with Android webviews and the BrowserRouter. Probably fine now, but worth giving extra care to testing Oculus Go.
There was a problem hiding this comment.
note this is just for dev
|
|
||
| // We may need to go to a new path after saving. | ||
| if (this.props.location.state.postPushPath) { | ||
| this.props.history.push(this.props.location.state.postPushPath); |
There was a problem hiding this comment.
Hmm.. This goBack + conditional push feels odd. Being able to hit back to close the modal is maybe nice but I am not sure I expect to be able to go back to re-open a modal.
src/react-components/ui-root.js
Outdated
| finished: "finished" | ||
| }; | ||
| // This needs to be updated as we add modal routes. | ||
| const MODAL_ROUTES = [ |
There was a problem hiding this comment.
Could alternatively define this as a state on the route or for bonus points make a ModalRoute component that does so, so you define this property as part of defining the route.
| @@ -228,17 +270,17 @@ class UIRoot extends Component { | |||
| onContinueAfterSignIn(); | |||
| }; | |||
|
|
|||
| this.showDialog(SignInDialog, { | |||
| this.showNonHistoriedDialog(SignInDialog, { | |||
There was a problem hiding this comment.
Like I said above I would vote all the modals are non historied except that hitting back can close them. Seems simpler than having 2 different types.
src/react-components/ui-root.js
Outdated
| if (success) { | ||
| this.props.history.push("/"); | ||
| } else { | ||
| this.props.history.goBack(); |
There was a problem hiding this comment.
This means I will be able to go "forward" into the dialog, not sure that is desirable.
src/react-components/ui-root.js
Outdated
| // This makes it so if you create an object, back will re-show the create object dialog, | ||
| // but if you cancel, it will not. | ||
| if (success) { | ||
| this.props.history.push("/"); |
There was a problem hiding this comment.
Again, not sure I really desire to hit back to re-open the dialog. I feel like if I am looking at a hubs room, hitting back should take me back to the lobby, if I am looking at a modal, it should close the modal.
|
…by just calling goBack when a modal is closed
|
Merging in #843 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a back button to the entry flow and migrates the room page to use the HTML history API for all the various UI states.
This PR adds
react-routerand wires up the main room page to use routes. In dev mode we use hash history in prod it uses the real HTML history API.Some notes:
If you refresh the page during the entry flow, we don't want to start you off in the middle of it, so there's a special case that just resets the path to "/" in that situation
If you enter the room and hit the back button, we don't want the entry flow to re-appear, and we also don't want you to end up on irrelevant URLs (eg the audio setup URL.) To get around this, if you hit back and end up being inside of the entry flow URL stack, the client just blows past it all and goes to the last history entry that was there before you landed on the page.
All the modal dialogs are now entered and exited using HTML history, except for the sign in flow, since we don't want to have the back button re-enter the sign in flow.
For modals, there's now the concept of the modal being successful or not (ie, was it not cancelled/closed with the X).
When a modal is closed with the X, we navigate back in the browser history (so the modal is effectively not going to appear again if you hit back) but if the modal is successful, then we push
/back onto the stack, so the back button will re-show it. (Note: you will currently always be brought to/, not what you were on before the modal appeared -- right now isn't relevant in the current flows since the only modal this happens for is the create dialog (and you are always at/before that) but we may need to revisit this.) I'm not convinced this is the ideal behavior, but the scenario I am thinking of is when we have a scene/avatar/object browser and you hit OK, the back button probably should bring you back into the browser.One somewhat negative side effect of doing this using
react-routeridomatically results in us building virtual dom elements for all the non-visible dialogs. (Whereas before we would short circuit building these elements using our own internal state flags.) I'm not sure we care.I also fixed up
vr_entry_typeto seemingly work better with_nowagain.