Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Comment component #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grandchild
Copy link
Contributor

See previous discussion on #41.
(Branch typo fixed, hence new PR)

@grandchild grandchild mentioned this pull request Jun 23, 2018
69 tasks
@azeem
Copy link
Owner

azeem commented Jun 23, 2018

I agree with your arguments. I'm not suggesting that we don't support comment component at all. I think we might want to handle unsupported components similarly ie. have them in the component tree but be inert in the rendering. So that you can still use (from the editor for e.g.) the component tree manipulation methods addComponent detachComponent etc. to move these inert components around in the tree, but still have no effect in the rendering.

So what i suggest is this:

  1. create a new Inert component class that doesn't do anything. Very similar to the Comment class you have right now, but with no concrete options interface.
  2. Here in Container.ts when a component class is not found, in addition to the warning we'll create an Inert component with given options and insert into tree as usual.
  3. create a new Comment component class that inherits from Inert and has the Comment options interface.
  4. Here in Effectlist.ts add an extra check for !(component instanceof Inert) to skip drawing for inert components as well.

@grandchild
Copy link
Contributor Author

That makes perfect sense.This way seems the best. Should I do it, or will you? I'm not 100% sure how to do the Inert subclass, but could figure it out for sure.

@azeem
Copy link
Owner

azeem commented Jun 24, 2018

If you're up for it. You can just continue adding to this PR, you already have most of it. Happy to help if you have any questions.

@grandchild grandchild force-pushed the components/comment branch 3 times, most recently from 63cdc3f to ccbc93e Compare June 27, 2018 22:45
@grandchild
Copy link
Contributor Author

I tried to write a unit test for either Inert or Comment but couldn't quickly figure out how to check that draw() would not be called, since the test already fails (rightly so) on the call to init().

@grandchild
Copy link
Contributor Author

Ah I screwed up again. I'll just not have init() throw an error, but just be empty.

@grandchild grandchild force-pushed the components/comment branch from ccbc93e to efaf503 Compare June 27, 2018 23:01
Copy link
Owner

@azeem azeem left a comment

Choose a reason for hiding this comment

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

Please feel free to commit WIP tests if you'd like me to take a look at them. Thanks!

@@ -49,6 +50,10 @@ export default abstract class Container extends Component implements IContainer
continue;
}
const component = new componentClass(this.main, this, opts);
if (component instanceof Inert) {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry If I wasn't clear about this in my previous comment. We'll never have an Inert component in the preset so this if block will never run. What we really want to do here is to implicitly create Inert component when componentClass cannot be found in the registry. So we can just replace the continue; in the previous if block with something like componentClass = Inert;.

super(main, parent, opts);
}

public updateText() {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't really need this update handler or the text private property. The private text property is not used anywhere inside this class and it's not exposed to anyone so it has no purpose. The text option is however already maintained in the opts property. When Webvs clients update them with the set method on a component it is automatically updated.

The update handlers are a way for the component to react to changes in opts and update internal state, re-compile shaders etc.

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

Successfully merging this pull request may close these issues.

2 participants