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

Changed: Use EnterCommand's afterExecute event to alter it's behavior. #29

Merged
merged 6 commits into from
Sep 19, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 5 additions & 1 deletion src/headings.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
* For licensing, see LICENSE.md.
*/

import Feature from '../core/feature.js';
import HeadingsEngine from './headingsengine.js';

import Feature from '../core/feature.js';

import Model from '../ui/model.js';
import ListDropdownController from '../ui/dropdown/list/listdropdown.js';
import ListDropdownView from '../ui/dropdown/list/listdropdownview.js';

import Collection from '../utils/collection.js';

/**
Expand Down Expand Up @@ -65,6 +68,7 @@ export default class Headings extends Feature {
editor.editing.view.focus();
} );

// Register UI component.
editor.ui.featureComponents.add( 'headings', ListDropdownController, ListDropdownView, dropdownModel );
}
}
7 changes: 5 additions & 2 deletions src/headingscommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,14 @@ export default class HeadingsCommand extends Command {
}

/**
* Executes the command if it is enabled.
* Executes command.
*
* @protected
* @param {String} [formatId] The identifier of the heading format that should be applied. It should be one of the
* {@link headings.HeadingsFormat heading formats} provided to the command constructor. If this parameter is not provided,
* the value from {@link headings.HeadingsCommand#defaultFormat defaultFormat} will be used.
* @returns {Object} data Data object, available in {@link core.Command#event:afterExecute}
* @returns {engine.model.Batch} data.batch Batch created and used by the command.
Copy link
Member

Choose a reason for hiding this comment

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

Leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I fixed it.

*/
_doExecute( formatId = this.defaultFormat.id ) {
// TODO: What should happen if format is not found?
Expand Down Expand Up @@ -134,7 +137,7 @@ export default class HeadingsCommand extends Command {
* @returns {headings.HeadingsFormat}
*/
_getFormatById( id ) {
return this.formats.find( item => item.id === id );
return this.formats.find( item => item.id === id ) || this.defaultFormat;
}
}

Expand Down
15 changes: 15 additions & 0 deletions src/headingsengine.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,20 @@ export default class HeadingsEngine extends Feature {
// Register command.
const command = new HeadingsCommand( editor, formats );
editor.commands.set( 'headings', command );

// If Enter Command is added to the editor, alter it's behavior.
const enterCommand = editor.commands.get( 'enter' );

if ( enterCommand ) {
this.listenTo( enterCommand, 'afterExecute', ( evt, data ) => {
const positionParent = editor.document.selection.getFirstPosition().parent;
const batch = data.batch;
const isHeading = formats.some( ( format ) => format.id == positionParent.name );

if ( isHeading && positionParent.name != command.defaultFormat.id && positionParent.childCount === 0 ) {
batch.rename( positionParent, command.defaultFormat.id );
}
} );
}
}
}
13 changes: 11 additions & 2 deletions tests/headingscommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ const formats = [
];

describe( 'HeadingsCommand', () => {
let editor, document, command, root;
let editor, document, command, root, schema;

beforeEach( () => {
return ModelTestEditor.create()
.then( newEditor => {
editor = newEditor;
document = editor.document;
command = new HeadingsCommand( editor, formats );
const schema = document.schema;
schema = document.schema;

for ( let format of formats ) {
schema.registerItem( format.id, '$block' );
Expand Down Expand Up @@ -53,6 +53,15 @@ describe( 'HeadingsCommand', () => {
expect( command.value ).to.equal( format );
} );
}

it( 'should be equal to defaultFormat if format has not been found', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why can't it be null? Why is this change needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because when you focus selection inside a model element that is not in formats array, you get this error:
Uncaught TypeError: Cannot read property 'label' of undefined.

This is because no option has been found to be displayed in the dropdown.

However, as I think of it now, it should probably be done like it is in CKE4. There is "Format" option there whenever you are inside something that is not recognized by Headings feature. Right now, in CKE5, you will have "Paragraph" option displayed. Because of that you are unable to actually change to the paragraph using Headings dropdown.

I think this is worthy of seperate issue because then we have to ask ourselves what is the defaultValue -- still paragraph or maybe "format"?

Copy link
Member

Choose a reason for hiding this comment

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

OK, so I'm fine with defaulting to the default format in such a case. But next time please do not squeeze such changes without mentioning them ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry, this is just the way I are.

schema.registerItem( 'div', '$block' );
setData( document, '<div>xyz</div>' );
const element = root.getChild( 0 );
document.selection.addRange( Range.createFromParentsAndOffsets( element, 1, element, 1 ) );

expect( command.value ).to.equal( command.defaultFormat );
} );
} );

describe( '_doExecute', () => {
Expand Down
22 changes: 21 additions & 1 deletion tests/headingsengine.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import HeadingsEngine from '/ckeditor5/headings/headingsengine.js';
import Paragraph from '/ckeditor5/paragraph/paragraph.js';
import VirtualTestEditor from '/tests/core/_utils/virtualtesteditor.js';
import HeadingsCommand from '/ckeditor5/headings/headingscommand.js';
import Enter from '/ckeditor5/enter/enter.js';
import { getData } from '/tests/engine/_utils/model.js';

describe( 'HeadingsEngine', () => {
let editor, document;

beforeEach( () => {
return VirtualTestEditor.create( {
features: [ HeadingsEngine ]
features: [ Enter, HeadingsEngine ]
} )
.then( newEditor => {
editor = newEditor;
Expand Down Expand Up @@ -72,4 +73,23 @@ describe( 'HeadingsEngine', () => {
expect( getData( document, { withoutSelection: true } ) ).to.equal( '<heading3>foobar</heading3>' );
expect( editor.getData() ).to.equal( '<h4>foobar</h4>' );
} );

it( 'should make enter command insert a defaultFormat block if selection ended at the end of heading block', () => {
editor.setData( '<h2>foobar</h2>' );
document.selection.collapse( document.getRoot().getChild( 0 ), 'end' );

editor.execute( 'enter' );

expect( getData( document ) ).to.equal( '<heading1>foobar</heading1><paragraph>[]</paragraph>' );
} );

it( 'should not alter enter command if selection not ended at the end of heading block', () => {
// This test is to fill code coverage.
editor.setData( '<h2>foobar</h2>' );
document.selection.collapse( document.getRoot().getChild( 0 ), 3 );

editor.execute( 'enter' );

expect( getData( document ) ).to.equal( '<heading1>foo</heading1><heading1>[]bar</heading1>' );
} );
} );