Modernize bgcolor #294
Modernize bgcolor #294
Conversation
|
@vibraphone why the css contains style/classes for other components? model.container.querySelector('.bg-color-container').style.backgroundColor = model.color; What was the API change that you wanted? ;-) |
|
@jourdain d3 is in there because I'm comfy with it. I will clean up the CSS. The API change I really wanted was what's mentioned in the commit: adopting the |
@jourdain d3 is in there because I'm comfy with it. I will clean up the CSS. The API change I really wanted was what's mentioned in the commit: adopting the |
vibraphone
force-pushed
the
modernize-bgcolor
branch
from
6a9580a
to
7c10355
Compare
|
@jourdain I've removed d3 and cleaned up the CSS. |
| return; | ||
| } | ||
|
|
||
| model.clientRect = model.container.getBoundingClientRect(); |
There was a problem hiding this comment.
that method could remain empty...
There was a problem hiding this comment.
I would like the method to be there (so that, for instance, it is easier to copy the component as a starting point for new components). Is it OK for the method to exist but do nothing? Or should I find something "demo-like" for it to do on resize to justify its existence?
There was a problem hiding this comment.
Yes the method should exist, but do nothing. You can put its content inside a comment so other component could use the actual content by uncommenting it...
There was a problem hiding this comment.
when using it as a template/starting-point.
|
LGTM otherwise |
| const green = new BGColorComponent({ color:'green' }); | ||
| const red = new BGColorComponent({ color:'red' }); | ||
| const blue = new BGColorComponent({ color:'blue' }); | ||
| const pink = new BGColorComponent({ color:'pink' }); |
There was a problem hiding this comment.
should this one use 'new' when the other example use 'newInstance'?
There was a problem hiding this comment.
good catch Aron, that should be newInstance() now...
There was a problem hiding this comment.
Thanks, Aron. Should be fixed now.
vibraphone
force-pushed
the
modernize-bgcolor
branch
from
7c10355
to
eea993a
Compare
|
@jourdain @aronhelser Thanks for the feedback. |
|
Thinking about it, you should have a destroy() method that call setContainer(null) and call the "parent" destroy. |
Now `BackgroundColor` provides a `newInstance()` method that takes publicAPI and model objects, making it a good minimal example. It also demonstrates how to import HTML fragments and CSS style from external files. See the `ToggleControl` and `Workbench` examples for usage.
vibraphone
force-pushed
the
modernize-bgcolor
branch
from
eea993a
to
858038b
Compare
No description provided.