Improve knockout usage with the Cesium Inspector #4857
Conversation
| @@ -104,247 +104,243 @@ define([ | |||
| * @type {Boolean} | |||
| * @default false | |||
| */ | |||
| this.frustums = false; | |||
| this.frustums = knockout.observable(false); | |||
There was a problem hiding this comment.
We use the knockout es5 plugin instead of creating observables directly so users can access observable properties the same way they would regular ones. So instead of creating the observable here, knockout.track creates all the observables behind the scenes.
| * @type {String} | ||
| * @default '1 of 1' | ||
| */ | ||
| this.depthFrustumText = knockout.observable('1 of 1'); |
There was a problem hiding this comment.
I would default this to ''
| that._scene.debugShowFrustums = false; | ||
| } | ||
| return true; | ||
| this.frustums.subscribe(function(val) { |
There was a problem hiding this comment.
Any time we do a subscribe, we need to do dispose of it when the widget is destroyed. So do something like
this._frustumsSubscription = this.frustums.subscribe(...)
then in CesiumInspectorViewModel.prototype.detroy do this._frustumsSubscription.dispose()
There was a problem hiding this comment.
Same for the subscriptions below
| @@ -434,13 +444,12 @@ define([ | |||
| return true; | |||
| }); | |||
|
|
|||
|
|
|||
| eventHandler.setInputAction(selectTile, ScreenSpaceEventType.LEFT_CLICK); | ||
| } else { | ||
| eventHandler.removeInputAction(ScreenSpaceEventType.LEFT_CLICK); | ||
| } | ||
| }); | ||
|
|
||
| knockout.track(this); |
There was a problem hiding this comment.
Change this back to use the second argument to define which properties we want to convert to observables. We don't want it to use every property on this
|
Good start here @austinEng! That clears up a lot of the weirdness. |
austinEng
force-pushed
the
cesiumInspector
branch
from
54a1cf3
to
6496d2c
Compare
|
Thank you for the feedback @hpinkos. Updated. |
| that._performanceDisplay = new PerformanceDisplay({ | ||
| container : that._performanceContainer | ||
| }); | ||
| } else { | ||
| that._performanceContainer.innerHTML = ''; | ||
| } | ||
| return true; | ||
| }); | ||
|
|
||
| this._showPrimitiveBoundingSphere = createCommand(function() { |
There was a problem hiding this comment.
This isn't being used anymore, right?
There was a problem hiding this comment.
Oh, nevermind. I see it's being used in multiple places, just not as a knockout binding.
|
@pjcozzi do we need to worry about breaking changes here, or can we just make a note in |
|
No concern about breaking changes. |
|
Great work here @austinEng, thanks! |
Fixes #3747