i/5692: The main editor toolbar should respect the config.toolbar.shouldGroupWhenFull configuration

[oleq]

Suggested merge commit message (convention)

Feature: The main editor toolbar should respect the config.toolbar.shouldGroupWhenFull configuration (see ckeditor/ckeditor5#5692).

---

Additional information

Requires ckeditor/ckeditor5-core#201.

[coveralls]

Coverage remained the same at 100.0% when pulling b1da1ac on i/5692 into b5e4793 on master.

[oleq]

TBH I hate this bit. I should've used this.config.define( 'toolbar.shouldGroupWhenFull', true ) in this constructor() instead. But...

The thing is that toolbar configuration (as in the Config instance) can be either as

• toolbar: [ 'bold', ... ]
• or toolbar: { items: [ 'bold', ... ], otherOptions: ... }
  at this stage.

Doing this.config.define( 'toolbar.shouldGroupWhenFull', true ) overrides the former format (the later is OK) creating toolbar: { shouldGroupWhenFull: true } and nuking the items configuration passed to the create().

So to keep things working I'd have to do something like this:

// Get the second "normalized" form in the Config first.
this.config.set( 'toolbar', normalizeToolbarConfig( this.config.get( 'toolbar' ) ) );
// Then set some default.
this.config.define( 'toolbar.shouldGroupWhenFull', true )

which I hate even more because it changes the internal form (as in the Config instance, not the plain object provided by the user) of the toolbar configuration and who knows with what consequences.

So yeah... maybe I'm overreacting but I think we should discuss this bit.

[Reinmar]

I agree... that sucks. You can report a followup and comment it here with a link.

Also, I'd move the condition outside of the ClassicEditorUIView() constructor call to have the correct value directly in shouldGroupWhenFull variable.

[Reinmar]

Also, see https://github.com/ckeditor/ckeditor5-core/pull/201/files#r347772936.

[Reinmar]

As commented.