Viewmodel phosphor widget implementation #4
Conversation
jasongrout
changed the title
| 'use strict'; | ||
|
|
||
| import { | ||
| ISignal, Signal |
jasongrout
force-pushed
the
viewmodel
branch
2 times, most recently
from
e5fd283
to
f7e15b5
Compare
|
@blink1073 - I cleaned up a lot and I think I fixed each one of your comments. |
| ` | ||
| textModel.mimetype = 'text/x-python' | ||
| textModel.lineNumbers = true; | ||
| /* let inputVModel = new InputAreaViewModel(); |
There was a problem hiding this comment.
Yep....more like, "preview of work to come..."
jasongrout
force-pushed
the
viewmodel
branch
from
edb63b7
to
f133220
Compare
|
@jasongrout, are you ready to merge and iterate on this one too? |
| */ | ||
| constructor(model: ITextEditorViewModel) { | ||
| super(); | ||
| this.addClass('CodeMirrorWidget'); |
There was a problem hiding this comment.
Should be jp-CodeMirrorWidget.
There was a problem hiding this comment.
I didn't realize we had settled on a prefix. Sounds good.
|
@blink1073 - sure we can merge and iterate from here. |
| this.addClass('InputAreaWidget'); | ||
| this._model = model; | ||
| for (let key in InputAreaWidget.update) { | ||
| InputAreaWidget.update[key].call(this, model[key]); |
There was a problem hiding this comment.
This line is causing a build failure. It should be: InputAreaWidget.update[key].call(this, (model as any)[key]);
There was a problem hiding this comment.
👎 on this flavor of dynamic dispatch. It's not type-safe and IMHO is harder to follow.
There was a problem hiding this comment.
yep, this was one of the experiments, and I already decided to scrap it. I have some commits deleting it that I haven't pushed.
|
ping @blink1073 - I switched back to using a switch statement |
| this._editor.on('change', (instance, change) => { | ||
| this._model.text = this._editor.getDoc().getValue(); | ||
| }); | ||
| model.stateChanged.connect(this._modelUpdate, this); |
There was a problem hiding this comment.
My convention for naming signal handlers is onThingSignalName, so for this case it would be _onModelStateChanged. Not saying you should follow this, just letting you know my convention. This let's me look at a method name and immediately make a good guess as to what it's connected to.
|
Thanks for the feedback. |
| /** | ||
| * Change handler for model updates. | ||
| */ | ||
| protected _onModelStateChanged(sender: ITextEditorViewModel, args: IChangedArgs<any>) { |
There was a problem hiding this comment.
We've been using a leading _ for private only.
|
Are we ready to merge this then? |
Thanks to @sccolbert for the suggestion.
Interaction with the text should happen through the model. Also, make internal functions protected. Thanks to @sccolbert for the suggestion.
|
I rebased in order to remove the experimental js code, instead of moving it to a new directory. It's a lot cleaner that way, and the code is still in the history if we want to look at it. |
|
LGTM, merge? |
|
I'm ready for it to merge. We'll need to iterate on the tests. |
|
Yep, iterating, thanks! |
WIP Viewmodel phosphor widget implementation
jasongrout
changed the title
|
And now of course I see that I didn't update the readme nor did I delete the js tests... |
This implementation follows design discussions about designing the models.