webvs icon indicating copy to clipboard operation
webvs copied to clipboard

Add Comment component

Open grandchild opened this issue 6 years ago • 5 comments

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

grandchild avatar Jun 23 '18 02:06 grandchild

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.

azeem avatar Jun 23 '18 19:06 azeem

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.

grandchild avatar Jun 24 '18 20:06 grandchild

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.

azeem avatar Jun 24 '18 23:06 azeem

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 avatar Jun 27 '18 22:06 grandchild

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

grandchild avatar Jun 27 '18 22:06 grandchild