Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

add a RenderWidget #299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add a RenderWidget #299

wants to merge 2 commits into from

Conversation

sccolbert
Copy link
Member

No description provided.

@sccolbert
Copy link
Member Author

cc @ellisonbg

@sccolbert
Copy link
Member Author

cc @jasongrout

@ellisonbg
Copy link
Contributor

ellisonbg commented Jul 11, 2017 via email

@sccolbert
Copy link
Member Author

:patientlywaiting:

* An object which can be used as a model for a render widget.
*/
export
interface IRenderModel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Don't have a model at all on this class, and let the subclass define it.
  2. Make the model a simple T | null and have a protected 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In JLab we have stopped using the Widget suffix in classnames as it is redundant. Fine here unless we have a better idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants