Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added DocumentListStyle command #11232

Merged
merged 10 commits into from
Feb 9, 2022
Merged

Added DocumentListStyle command #11232

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bbac39b
Added DocumentListStyle command
arkflpc Feb 7, 2022
c63dac8
Converting to list and style application in single batch
arkflpc Feb 8, 2022
dfe7b36
Moved some checks out of listwalker
arkflpc Feb 8, 2022
6a84586
Code refactoring.
niegowski Feb 8, 2022
c63648a
Fix after code-review
arkflpc Feb 9, 2022
ab4baf4
Apply review comment.
niegowski Feb 9, 2022
76f2f50
Fix after code-review
arkflpc Feb 9, 2022
3eb6716
Fix after code-review
arkflpc Feb 9, 2022
2631672
Reordered imports.
niegowski Feb 9, 2022
fe4e747
Fix after code-review
arkflpc Feb 9, 2022
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
45 changes: 14 additions & 31 deletions packages/ckeditor5-list/src/documentlist/utils/listwalker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ export default class ListWalker {
* @param {Boolean} [options.sameItemId=false] Whether should return only blocks with the same `listItemId` attribute
* as the start element.
* @param {Boolean} [options.sameItemType=false] Whether should return only blocks wit the same `listType` attribute.
* @param {Bollean} [options.sameProperties=false] Whether should return only blocks with the same `listStyle`,
* `listReversed` and `listStart` attributes.
* @param {Array.<String>} [options.sameAttributes=[]] Additional attributes that must be the same for each block.
niegowski marked this conversation as resolved.
Show resolved Hide resolved
* @param {Boolean} [options.sameIndent=false] Whether blocks with the same indent level as the start block should be included
* in the result.
* @param {Boolean} [options.lowerIndent=false] Whether blocks with a lower indent level than the start block should be included
Expand Down Expand Up @@ -66,28 +65,16 @@ export default class ListWalker {
this._startItemType = startElement.getAttribute( 'listType' );

/**
* The `listStyle` of the start block.
* Additional attributes of the start block.
*
* @private
* @type {String|undefined}
* @type {Map}
*/
this._startItemStyle = startElement.getAttribute( 'listStyle' );
this._startAttributes = new Map();

/**
* The `listReversed` of the start block.
*
* @private
* @type {Boolean|undefined}
*/
this._startItemIsReversed = startElement.getAttribute( 'listReversed' );

/**
* The `listStart` of the start block.
*
* @private
* @type {Number|undefined}
*/
this._startItemStartIndex = startElement.getAttribute( 'listStart' );
for ( const attr of ( options.sameAttributes || [] ) ) {
this._startAttributes.set( attr, startElement.getAttribute( attr ) );
}

/**
* The iterating direction.
Expand Down Expand Up @@ -122,11 +109,12 @@ export default class ListWalker {
this._sameItemType = !!options.sameItemType;

/**
* Whether should return only blocks with the same `listStyle`, `listReversed` and `listStart` attributes.
* Additional attributes that must be the same for each block.
*
* @private
* @type {Boolean}
* @type {Array.<String>}
*/
this._sameProperties = !!options.sameProperties;
this._sameAttributes = options.sameAttributes || [];

/**
* Whether blocks with the same indent level as the start block should be included in the result.
Expand Down Expand Up @@ -163,8 +151,7 @@ export default class ListWalker {
* @param {Boolean} [options.sameItemId=false] Whether should return only blocks with the same `listItemId` attribute
* as the start element.
* @param {Boolean} [options.sameItemType=false] Whether should return only blocks with the same `listType` attribute.
* @param {Bollean} [options.sameProperties=false] Whether should return only blocks with the same `listStyle`,
* `listReversed` and `listStart` attributes.
* @param {Array.<String>} [options.sameAttributes=[]] Additional attributes that must be the same for each block.
* @param {Boolean} [options.sameIndent=false] Whether blocks with the same indent level as the start block should be included
* in the result.
* @param {Boolean} [options.lowerIndent=false] Whether blocks with a lower indent level than the start block should be included
Expand Down Expand Up @@ -243,12 +230,8 @@ export default class ListWalker {
break;
}

// Abort if item has different `listStyle`, `listStart` or `listReversed` properties.
if ( this._sameProperties && (
node.getAttribute( 'listStyle' ) != this._startItemStyle ||
node.getAttribute( 'listStart' ) != this._startItemStartIndex ||
node.getAttribute( 'listReversed' ) != this._startItemIsReversed
) ) {
// Abort if item has any additionaly specified attribute different.
if ( this._sameAttributes.some( attr => node.getAttribute( attr ) != this._startAttributes.get( attr ) ) ) {
break;
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/ckeditor5-list/src/documentlist/utils/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,18 @@ export function getNestedListBlocks( listItem ) {
* @returns {Array.<module:engine/model/element~Element>}
*/
export function getListItems( listItem ) {
const listAddidionalProperties = [ 'listReversed', 'listStart', 'listStyle' ];
niegowski marked this conversation as resolved.
Show resolved Hide resolved

const backwardBlocks = new ListWalker( listItem, {
sameIndent: true,
sameItemType: true,
sameProperties: true
sameAttributes: listAddidionalProperties
} );

const forwardBlocks = new ListWalker( listItem, {
sameIndent: true,
sameItemType: true,
sameProperties: true,
sameAttributes: listAddidionalProperties,
includeSelf: true,
direction: 'forward'
} );
Expand Down
37 changes: 20 additions & 17 deletions packages/ckeditor5-list/src/documentlistproperties/documentliststylecommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,30 @@ export default class DocumentListStyleCommand extends Command {
* @protected
*/
execute( options = {} ) {
this._tryToConvertItemsToList( options );

const model = this.editor.model;
const document = model.document;
let blocks = Array.from( document.selection.getSelectedBlocks() )
.filter( block => block.hasAttribute( 'listStyle' ) );

if ( !blocks.length ) {
return;
}

if ( document.selection.isCollapsed ) {
blocks = getListItems( blocks[ 0 ] );
} else {
blocks = expandListBlocksToCompleteItems( blocks, { withNested: false } );
}

model.change( writer => {
for ( const block of blocks ) {
writer.setAttribute( 'listStyle', options.type || this._defaultType, block );
}
this._tryToConvertItemsToList( options );

model.enqueueChange( writer.batch, writer => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be required, commands executed inside a change block will automatically be a part of the outermost change block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you needed this to make the following blocks pass the block.hasAttribute( 'listStyle' ) test that was added by the post-fixer after the first change block and before the enqueueChange block but you could check whether the selected blocks are list items at all.

let blocks = Array.from( document.selection.getSelectedBlocks() )
.filter( block => block.hasAttribute( 'listStyle' ) );

if ( !blocks.length ) {
return;
}

if ( document.selection.isCollapsed ) {
blocks = getListItems( blocks[ 0 ] );
} else {
blocks = expandListBlocksToCompleteItems( blocks, { withNested: false } );
}

for ( const block of blocks ) {
writer.setAttribute( 'listStyle', options.type || this._defaultType, block );
}
} );
} );
}

Expand Down
38 changes: 9 additions & 29 deletions packages/ckeditor5-list/tests/documentlist/utils/listwalker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ describe( 'DocumentList - utils - ListWalker', () => {
expect( blocks[ 1 ] ).to.equal( fragment.getChild( 1 ) );
} );

it( 'should return items of the same style', () => {
it( 'should return items of the same additional attributes (single specified)', () => {
const input = modelList( [
'* 0 {style:abc}',
'* 1',
'* 1 {start:5}',
'* 2 {style:xyz}'
] );

Expand All @@ -196,28 +196,7 @@ describe( 'DocumentList - utils - ListWalker', () => {
direction: 'forward',
sameIndent: true,
includeSelf: true,
sameProperties: true
} );
const blocks = Array.from( walker );

expect( blocks.length ).to.equal( 2 );
expect( blocks[ 0 ] ).to.equal( fragment.getChild( 0 ) );
expect( blocks[ 1 ] ).to.equal( fragment.getChild( 1 ) );
} );

it( 'should return items of the same start index', () => {
const input = modelList( [
'* 0 {start:5}',
'* 1',
'* 2 {start:6}'
] );

const fragment = parseModel( input, schema );
const walker = new ListWalker( fragment.getChild( 0 ), {
direction: 'forward',
sameIndent: true,
includeSelf: true,
sameProperties: true
sameAttributes: [ 'listStyle' ]
} );
const blocks = Array.from( walker );

Expand All @@ -226,19 +205,20 @@ describe( 'DocumentList - utils - ListWalker', () => {
expect( blocks[ 1 ] ).to.equal( fragment.getChild( 1 ) );
} );

it( 'should return items of the same reversal status', () => {
it( 'should return items of the same additional attributes (multiple specified)', () => {
const input = modelList( [
'* 0 {reversed:true}',
'* 1',
'* 2 {reversed:false}'
'* 0 {style:abc}',
'* 1 {start:5}',
'* 2 {reversed:true}',
'* 3 {style:xyz}'
] );

const fragment = parseModel( input, schema );
const walker = new ListWalker( fragment.getChild( 0 ), {
direction: 'forward',
sameIndent: true,
includeSelf: true,
sameProperties: true
sameAttributes: [ 'listStyle', 'listReversed' ]
} );
const blocks = Array.from( walker );

Expand Down
8 changes: 0 additions & 8 deletions packages/ckeditor5-list/tests/documentlistproperties/documentliststylecommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,31 +447,23 @@ describe( 'DocumentListStyleCommand', () => {
Foo.[]
` ) );

const modelChangeStub = sinon.stub( model, 'change' ).named( 'model#change' );

listStyleCommand.execute( { type: 'default' } );

expect( getData( model ) ).to.equalMarkup( modelList( `
Foo.[]
` ) );

expect( modelChangeStub.called ).to.equal( false );
} );

it( 'should not update anything if no listItem found in the selection (style no specified)', () => {
setData( model, modelList( `
Foo.[]
` ) );

const modelChangeStub = sinon.stub( model, 'change' ).named( 'model#change' );

listStyleCommand.execute();

expect( getData( model ) ).to.equalMarkup( modelList( `
Foo.[]
` ) );

expect( modelChangeStub.called ).to.equal( false );
} );
} );
} );