Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

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

Merged
merged 2 commits into from
Nov 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
932e90d
Feature: The main editor toolbar should respect the config.toolbar.sh…
oleq Nov 18, 2019
b1da1ac
Renamed config.toolbar.shouldGroupWhenFull to config.toolbar.shouldNo…
oleq Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/classiceditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ export default class ClassicEditor extends Editor {

this.model.document.createRoot();

this.ui = new ClassicEditorUI( this, new ClassicEditorUIView( this.locale, this.editing.view ) );
const shouldGroupWhenFull = this.config.get( 'toolbar.shouldGroupWhenFull' );
const view = new ClassicEditorUIView( this.locale, this.editing.view, {
shouldToolbarGroupWhenFull: shouldGroupWhenFull === undefined ? true : shouldGroupWhenFull
Copy link
Member Author

@oleq oleq Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} );

this.ui = new ClassicEditorUI( this, view );

attachToForm( this );
}
Expand Down
8 changes: 6 additions & 2 deletions src/classiceditoruiview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ export default class ClassicEditorUIView extends BoxedEditorUIView {
*
* @param {module:utils/locale~Locale} locale The {@link module:core/editor/editor~Editor#locale} instance.
* @param {module:engine/view/view~View} editingView The editing view instance this view is related to.
* @param {Object} [options={}] Configuration options fo the view instance.
* @param {Boolean} [options.shouldToolbarGroupWhenFull] When set `true` enables automatic items grouping
* in the main {@link module:editor-classic/classiceditoruiview~ClassicEditorUIView#toolbar toolbar}.
* See {@link module:ui/toolbar/toolbarview~ToolbarOptions#shouldGroupWhenFull} to learn more.
*/
constructor( locale, editingView ) {
constructor( locale, editingView, options = {} ) {
super( locale );

/**
Expand All @@ -46,7 +50,7 @@ export default class ClassicEditorUIView extends BoxedEditorUIView {
* @member {module:ui/toolbar/toolbarview~ToolbarView}
*/
this.toolbar = new ToolbarView( locale, {
shouldGroupWhenFull: true
shouldGroupWhenFull: options.shouldToolbarGroupWhenFull
} );

/**
Expand Down
24 changes: 24 additions & 0 deletions tests/classiceditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,30 @@ describe( 'ClassicEditor', () => {
expect( editor.ui ).to.be.instanceof( ClassicEditorUI );
expect( editor.ui.view ).to.be.instanceof( ClassicEditorUIView );
} );

describe( 'automatic toolbar items groupping', () => {
it( 'should be on by default', () => {
const editorElement = document.createElement( 'div' );
const editor = new ClassicEditor( editorElement );

expect( editor.ui.view.toolbar.options.shouldGroupWhenFull ).to.be.true;

editorElement.remove();
} );

it( 'can be disabled using config.toolbar.shouldGroupWhenFull', () => {
const editorElement = document.createElement( 'div' );
const editor = new ClassicEditor( editorElement, {
toolbar: {
shouldGroupWhenFull: false
}
} );

expect( editor.ui.view.toolbar.options.shouldGroupWhenFull ).to.be.false;

editorElement.remove();
} );
} );
} );
} );

Expand Down
22 changes: 20 additions & 2 deletions tests/classiceditoruiview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,26 @@ describe( 'ClassicEditorUIView', () => {
expect( view.stickyPanel.content.get( 0 ) ).to.equal( view.toolbar );
} );

it( 'has automatic items grouping enabled', () => {
expect( view.toolbar.options.shouldGroupWhenFull ).to.be.true;
describe( 'automatic items grouping', () => {
it( 'should be disabled by default', () => {
expect( view.toolbar.options.shouldGroupWhenFull ).to.be.undefined;
} );

it( 'should be controlled via options.shouldToolbarGroupWhenFull', () => {
const locale = new Locale();
const editingView = new EditingView();
const editingViewRoot = createRoot( editingView.document );
const view = new ClassicEditorUIView( locale, editingView, {
shouldToolbarGroupWhenFull: true
} );

view.editable.name = editingViewRoot.rootName;
view.render();

expect( view.toolbar.options.shouldGroupWhenFull ).to.be.true;

return view.destroy();
} );
} );
} );

Expand Down