T/10: Provide necessary UI components for Classic Creator #11
Conversation
…es. Code refactoring.
| marginLeft: -window.scrollX - 1 + 'px' | ||
| } ); | ||
|
|
||
| this.model.isSticky = true; |
There was a problem hiding this comment.
I don't like the view changing the model. This in fact isn't needed in the model at all. This is view's property (internal at this point).
There was a problem hiding this comment.
It is a thing of a model because of:
this.template.attributes.class.push( bind.if( 'isSticky', 'ck-toolbar_sticky' ) );There was a problem hiding this comment.
That's a super bad smell for me unfortunately ;/. If we need to add something to the model only to make bindings work, it's wrong.
There was a problem hiding this comment.
PS. Note that isSticky is a totally presentational property.
|
Beside these two issue which I commented, it's ready for merge. |
|
I suppose I also forgot about tests for |
|
Surprisingly, 0 tests means 100% CC :P. |
|
For me #11 (comment) still makes it R-. |
OK, we talked with @oleq that this needs a bigger refactoring in couple of existing components at once. We decided to try to implement MVVM in all components, because this pattern should fit best, but needs to be implement more consistently. So, for now we're going to ignore this issue. We'll address it later in https://github.com/ckeditor/ckeditor5-ui/issues/26 |
As in ckeditor/ckeditor5#135.
Closes #10.