Followup for the big UI library refactoring

[Reinmar]

Follow-up for https://github.com/ckeditor/ckeditor5-ui/issues/95.

ckeditor5-ui

• View#addChild -> View#addChildren and can't the param be an iterable? It's not consistent with e.g. engine.view.Element#insertChildren() and look bad here: https://github.com/ckeditor/ckeditor5-link/pull/58/files#diff-55321c76ff0d0fac681dcdaa3f17de75R60. (fixed by https://github.com/ckeditor/ckeditor5-ui/issues/108)
• View#bindTemplate requires some example. Or, in fact, there should be a more rich example (with bindings) in the class.
  • In fact... why isn't this method called bind or binder or sth? I'd try to find a better name for it cause the current one confused even me.
• I don't like that you bind an event using domEventName: bind.to( 'viewEventName' ). It's the opposite direction than data binding, so a separate function for that would be more clear (bind.fire()?... but then bind isn't the best word too). WDYT? Actually, maybe this deserves a bigger ticket because I also have a problem with reading bind.if(). Maybe someone will have a better idea for this API.

ckeditor5-ui-default

• General thing – merge multiple this.set() calls into one big this.set( { keys } ).

• EditorUI should be brought back (closed in Bring back EditorUI "controller" ckeditor5-ui-default#119)

  • It should IMO contain the component factory instance (which is now in EditorUIView) and contain the code needed to bootstrap the specific UI's view (if there's any special logic).
  • I think that that we'll know what to put in the controller by checking what needs to know about the editor. Currently, the view requires it but if we move it outside then you see that:
    • editor is needed for the feature components factory,
    • initializing the toolbar requires the editor (cause the buttons are in the config and feature components in the controller).

  So, we need to decide whether it's good that a view knows about the editor. If not (and I think so, though the reason is purely theoretical – it's the view's reusability) then EditorUI is needed. If not, then EditorUI may still be needed... :D.

  • Note: I'm unsure whether we need any base class for EditorUI. Perhaps it's enough to define an interface in the core (not ui-default!). This will allow easily documenting in the core what editor.ui is supposed to contain.
  • Related issue: https://github.com/ckeditor/ckeditor5-ui/issues/77.
  • If it won't be brought back, then at least the comment in ComponentFactory must be fixed.

(all below fixed by ckeditor/ckeditor5-ui-default#118)

1. ButtonView
   • Not related to this PR, but why is native click prevented only when button is not enabled?
2. EditorUIView
   • Why is #icons observable?
   • Remove #editor (after bringing back EditorUI).
3. EditableUIView
   • Why is #name defined here and not used (it's used in a child view only)?
4. IconManagerView
   • Why is #sprite observable?
5. StickyToolbarView
   • Shouldn't destroy() return a promise?
6. Handlers
   • They should be simplified. As we discussed already – constantly adding and removing listeners just make them more fragile and requires more code.

editor-classic

• Will require changes once EditorUI is brought back. ClassicEditorUIView shouldn't know about the editor. (closed in https://github.com/ckeditor/ckeditor5-editor-classic/issues/38)

basic-styles

• https://github.com/ckeditor/ckeditor5-basic-styles/pull/32/files#diff-e1ae04aa196b2e58d217887d5c184135R40 and https://github.com/ckeditor/ckeditor5-basic-styles/pull/32/files#diff-a1955eb538778e28166800d49a7bbd88R40 – isOn and isEnabled aren't needed here cause they are set below.

(fixed by https://github.com/ckeditor/ckeditor5-basic-styles/issues/35)

heading

• https://github.com/ckeditor/ckeditor5-heading/pull/42/files#diff-dd7f09251ed58d70ff2596aaaa063701R49 – isOn and isEnabled aren't needed (cause they equal their defaults).

(fixed by https://github.com/ckeditor/ckeditor5-heading/issues/48)

undo

• https://github.com/ckeditor/ckeditor5-undo/pull/46/files#diff-0af576b118e4e50973341c83f7ecd7f9R161 – not necessary cause equals the default.

(fixed by https://github.com/ckeditor/ckeditor5-undo/issues/49)

link

1. LinkFormView
   • Documentation of cancel and unlink events is missing.
   • https://github.com/ckeditor/ckeditor5-link/pull/58/files#diff-55321c76ff0d0fac681dcdaa3f17de75R141 can't we delegate this?
   • Why are we using a form finally?

(fixed by https://github.com/ckeditor/ckeditor5-link/issues/63)

[Reinmar]

PS. Please mind that I made a broader review of the UI than just focusing on what has changed. It's the right time I feel to also clear some other things like the bindings.

[pjasiun]

1. Bring EditorUI (controller) back: Bring EditorUI (controller) back ckeditor5-ui-default#109
2. Why both editor and locale are needed to create EdiorView or BoxedEditorView (https://github.com/ckeditor/ckeditor5-ui-default/blob/011390be10dc40ac1e2358311e4b0668e71bebbc/src/editorui/editoruiview.js#L27)? locale can be taken for the editor.locale. If we want to make locale obligated for every view it should be always the first parameter, so here the order should be locale, editor.
3. not sure if it is a regression: https://github.com/ckeditor/ckeditor5-ui/issues/105
4. regression: https://github.com/ckeditor/ckeditor5-ui/issues/92#issuecomment-259464972

[oskarwrobel]

2.View#bindTemplate requires some example. Or, in fact, there should be a more rich example (with > bindings) in the class.
In fact... why isn't this method called bind

It was called bind but we had to change it because of collision with ObservableMixin#bind.

3.ButtonView

Not related to this PR, but why is native click prevented only when button is not enabled?

It is to prevent form submit. Related discussion: https://github.com/ckeditor/ckeditor5-theme-lark/issues/14#issuecomment-244999963

8.Handlers

They should be simplified. As we discussed already – constantly adding and removing listeners just make them more fragile and requires more code.

I don't agree that it makes them more fragile. It is a quite bulletproof solution. But yes, it makes it more complicated and requires more code.

[oleq]

It was called bind but we had to change it because of collision with ObservableMixin#bind.

👍

[Reinmar]

Right, makes sense. But I also asked more generally whether we find this API clear. Like:

I don't like that you bind an event using domEventName: bind.to( 'viewEventName' ). It's the opposite direction than data binding, so a separate function for that would be more clear

Perhaps it'd be worth trying to find a better names for all this. "bindTemplate" is a verb+noun so it sounds like an action... but it's an object. Then, you don't "bindTemplate.to" a thing. You bind a specific template property to an observable property and you delegate a DOM event to a view event.

We'll never make this API a natural language, but perhaps we can work on making it as close to that as possible. This requires a lot of creativity and I'm not able to propose something complete so I raised this as an issue.

[Reinmar]

In https://github.com/ckeditor/ckeditor5-engine/issues/678 I mentioned that perhaps the editor UI controller should simply be called UIController. In order to have a consistent API let's not reintroduce it before we have that issue settled.

[oskarwrobel]

1.General thing – merge multiple this.set() calls into one big this.set( { keys } ).

What is the difference? One big this.set( {} ) is a loop through properties and call
this.set( 'key', val ) for each.

I'm just curious why you prefer to merge it into one set. For me this looks better

        /**
         * Some docs.
         */
        this.staticProperty = 'value';

        /**
         * Some docs.
         */
        this.otherStaticProperty = 'value';

        /**
         * Some docs.
         */
        this.set( 'observableProperty',  'value' );

        /**
         * Some docs.
         */
        this.set( 'otherObservableProperty', 'value' );

than

        /**
         * Some docs.
         */
        this.staticProperty = 'value';

        /**
         * Some docs.
         */
        this.otherStaticProperty = 'value';

        this.set( {
            /**
             * Some docs.
             */
            observableProperty: 'value',

            /**
             * Some docs.
             */
            otherObservableProperty: 'value'
        } );

[Reinmar]

I don't know. I wrote it down and immediately thought that it's a premature optimisation (code size). So in fact, I'm against this :D.