add a RenderWidget #299
add a RenderWidget #299
Conversation
|
cc @ellisonbg |
|
cc @jasongrout |
|
Have some feedback before it is merged
…Sent from my iPhone
On Jul 11, 2017, at 4:57 PM, Steven Silvester ***@***.***> wrote:
@blink1073 approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
:patientlywaiting: |
| * An object which can be used as a model for a render widget. | ||
| */ | ||
| export | ||
| interface IRenderModel { |
There was a problem hiding this comment.
Nice, this was the main point I wanted to make - that this should be a simple interface with only the signal. This will allow us to use it with our Redux style state stores as well. Is the name of the signal the same in both places?
There was a problem hiding this comment.
The name is not the same in both places, but your model is likely to wrap the store anyway.
We were talking about a few alternatives:
- Don't have a model at all on this class, and let the subclass define it.
- Make the model a simple
T | nulland have aprotected modelSwapped(old, new)method which gets called, and let the subclass subscribe/unsubscribe as/if needed to their model type.
| * method. Advanced use cases may reimplement some of the other methods. | ||
| */ | ||
| export | ||
| abstract class RenderWidget<T extends IRenderModel = {}> extends Widget { |
There was a problem hiding this comment.
In JLab we have stopped using the Widget suffix in classnames as it is redundant. Fine here unless we have a better idea.
There was a problem hiding this comment.
I've been trying to keep widget names to two words, so Render is clearly out. Not sure I have a better idea. We have overloaded the term "renderer" too much between Phosphor and JLab for me to use it here.
No description provided.