Editor camera state cleanup #2701
Conversation
| const currentOrbit = cameraState.orbit; | ||
| this.cameraOrbitEditor.yawInput.value = currentOrbit.thetaDeg; | ||
| this.cameraOrbitEditor.pitchInput.value = currentOrbit.phiDeg; | ||
| this.cameraOrbitEditor.style.display = ''; |
There was a problem hiding this comment.
Question: what is the difference of having display set to none vs ''?
There was a problem hiding this comment.
setting an attribute to '' mean unset, so it now just inherits its behavior from its parent HTML elements. I could set it to 'block', but this is safer, since I don't have to worry about what it's originally computed style was. In this context, '' means show.
| @query('#limit-enabled') enabledInput?: HTMLInputElement; | ||
| @query('#minimum') minimumInput!: SliderWithInputElement; | ||
| @query('#maximum') maximumInput!: SliderWithInputElement; | ||
| @query('#limit-enabled') enabledInput!: HTMLInputElement; |
There was a problem hiding this comment.
What was the reasoning behind the original design?
It looks like previously it was ok for any of these variables to be undefined, and now we are saying it shouldn't?
There was a problem hiding this comment.
Yeah, I've been slowly updating this style across the editor. Before when a panel was disabled, they replaced the html wholesale with nothing, while now I'm preferring to set display: 'none'. In this way the elements always exist and a lot of checking can be removed. In some cases, they had ? even if they already always existed, probably because this is the TS default.
| this.cameraSnippet = getCamera(state); | ||
| const config = getConfig(state); | ||
| this.poster = config.poster; | ||
| this.initialCamera = config.cameraOrbit ?? 'auto auto auto'; |
There was a problem hiding this comment.
In js world they love to use all the possible signs lol ?? == === !=== =D
| // changes to the <model-viewer> element in case they need to be queried by | ||
| // subsequent components. Keep in mind that those will still have to await | ||
| // updateComplete since rendering is async in Lit Element. | ||
| import './components/model_viewer_preview/model_viewer_preview.js'; |
There was a problem hiding this comment.
Does the model_viewer_preview actually expose API to make changes to the model? Just trying to understand why it's called preview but its applying changes.
There was a problem hiding this comment.
The preview is where the model-viewer instance lives, so yes, it effectively exposes the whole MV API to the editor features.
There was a problem hiding this comment.
Other components only use getters on the MV element. The preview is the one place where the config Redux state is applied to the MV element, which happens when its render method is called.
| <me-draggable-input | ||
| id="pitch" | ||
| innerLabel="pitch" | ||
| value=${this.orbit.phiDeg} |
There was a problem hiding this comment.
Now that you're removing these values. Will they be wrong when the model is initially loaded, and only be correct once you start dragging?
There was a problem hiding this comment.
Yeah, they aren't shown initially and I decided it quite a bit simpler to make them just sync one way. There are edge cases where you can update the snippet and it won't show in the UI, but I think this still overall has fewer bugs than where we were.
| return { | ||
| ...state, poster: action.payload | ||
| } | ||
| return {...state, poster: action.payload}; |
There was a problem hiding this comment.
I like the expansion syntax, not sure if this was a thing the last time got into javascript.
| } | ||
| } | ||
|
|
||
| /** Use when the user wants to load a new config (probably from a snippet). */ |
There was a problem hiding this comment.
This reminds me, something we don't have a test for is ensuring the flow of when a user loads a model, then goes to edit the snippet manually, and then saves that. Just wanted to make sure that functionality was still preserved and caught somewhere.
3 participants
Follow-on to the big refactor that removed material state from the editor (relying instead on model-viewer's own state), this smaller refactor removes the camera state from Redux as its information was already contained in the model-viewer config state in Redux. It also fixes a number of async bugs related to updating the UI, especially noticeable with the camera target.