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.

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

List style feature #7861

Merged
merged 39 commits into from
Aug 21, 2020
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
9b24335
Added manual tests for the list styles feature.
pomek Aug 7, 2020
b6fa8b9
An initial implementation for the list styles feature.
pomek Aug 7, 2020
3591a8d
Tests init.
pomek Aug 7, 2020
f3df005
An initial implementation of ListStyleCommand.
pomek Aug 10, 2020
a40d46e
The "listStyle" attribute should be removed when renaming listItem to…
pomek Aug 10, 2020
5a3e307
The IndentCommand could return nodes that were modified.
pomek Aug 10, 2020
34b0e5c
Many improvements with integration List style feature with other plug…
pomek Aug 10, 2020
8f5ff69
Added tests for ListStylesCommand, added integrations with other plug…
pomek Aug 12, 2020
b42f5c7
Restored a fancy test.
pomek Aug 12, 2020
05837c9
Minor improvements.
pomek Aug 12, 2020
31e34de
Additional improvements.
pomek Aug 15, 2020
bafb120
Allowed overriding existing components when adding new ones to the co…
oleq Aug 14, 2020
b889ae0
Added icons for list styles.
oleq Aug 14, 2020
3f14175
Added CSS styles for the list styles UI.
oleq Aug 14, 2020
c0d4212
Implemented the ListStylesUI plugin.
oleq Aug 14, 2020
13c418b
Tests: Added tests for the ListStylesUI class.
oleq Aug 14, 2020
4a3254b
Added tooltips to list style buttons.
oleq Aug 14, 2020
ebf5438
Added translation entries to contexts.json.
oleq Aug 14, 2020
b8eeaeb
Fixed a typo in the package.json file.
oleq Aug 14, 2020
3d35fbf
Removed invalid change.
pomek Aug 17, 2020
f312825
Removed invalid change.
pomek Aug 17, 2020
f8faee1
Clean.
pomek Aug 18, 2020
87beb07
Merge pull request #7845 from ckeditor/i/7803-list-styles-ui
pomek Aug 18, 2020
d3b2f9a
Removed "custom" UI from the list styles manual test.
pomek Aug 18, 2020
b1064e9
Fixed the feature name. [skip ci]
pomek Aug 19, 2020
c31d38d
Fixed a link in API docs. [skip ci]
pomek Aug 19, 2020
d8cf02d
Renamed the feature.
pomek Aug 19, 2020
879d47d
Renamed an internal function.
pomek Aug 19, 2020
311f3a2
The single undo step will be created while selection was not anchored…
pomek Aug 19, 2020
178ec10
Added basic styles to the manual test.
pomek Aug 19, 2020
eaed650
Use default value obtained from the command.
pomek Aug 19, 2020
0143084
Renamed the "executeCleanup" to "_executeCleanup". Now the event is m…
pomek Aug 19, 2020
39f0604
Update forward indent command suite name.
jodator Aug 20, 2020
ac39cef
Changed a way that items for updating are obtained.
pomek Aug 20, 2020
373ad4c
Merge branch 'master' into i/7801
pomek Aug 21, 2020
466d244
Marked "_executeCleanup" event as protected.
pomek Aug 21, 2020
c7e2355
Added a test for changes introduced in ac39cef.
pomek Aug 21, 2020
7c93510
Clean up.
pomek Aug 21, 2020
b3d5eb0
List style is not a format attribute.
pomek Aug 21, 2020
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
22 changes: 21 additions & 1 deletion packages/ckeditor5-list/lang/contexts.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,25 @@
{
"Numbered List": "Toolbar button tooltip for the Numbered List feature.",
"Bulleted List": "Toolbar button tooltip for the Bulleted List feature.",
"To-do List": "Toolbar button tooltip for the To-do List feature."
"To-do List": "Toolbar button tooltip for the To-do List feature.",
"Bulleted list styles toolbar": "The ARIA label of the toolbar displaying buttons allowing users to change the bulleted list style.",
"Numbered list styles toolbar": "The ARIA label of the toolbar displaying buttons allowing users to change the numbered list style.",
"Toggle the disc list style": "The ARIA label of the button that toggles the \"disc\" list style.",
"Toggle the circle list style": "The ARIA label of the button that toggles the \"circle\" list style.",
"Toggle the square list style": "The ARIA label of the button that toggles the \"square\" list style.",
"Toggle the decimal list style": "The ARIA label of the button that toggles the \"decimal\" list style.",
"Toggle the decimal with leading zero list style": "The ARIA label of the button that toggles the \"decimal with leading zero\" list style.",
"Toggle the lower–roman list style": "The ARIA label of the button that toggles the \"lower–roman\" list style.",
"Toggle the upper–roman list style": "The ARIA label of the button that toggles the \"upper–roman\" list style.",
"Toggle the lower–latin list style": "The ARIA label of the button that toggles the \"lower–latin\" list style.",
"Toggle the upper–latin list style": "The ARIA label of the button that toggles the \"upper–latin\" list style.",
"Disc": "The tooltip text of the button that toggles the \"disc\" list style.",
"Circle": "The tooltip text of the button that toggles the \"circle\" list style.",
"Square": "The tooltip text of the button that toggles the \"square\" list style.",
"Decimal": "The tooltip text of the button that toggles the \"decimal\" list style.",
"Decimal with leading zero": "The tooltip text of the button that toggles the \"decimal with leading zero\" list style.",
"Lower–roman": "The tooltip text of the button that toggles the \"lower–roman\" list style.",
"Upper-roman": "The tooltip text of the button that toggles the \"upper–roman\" list style.",
"Lower-latin": "The tooltip text of the button that toggles the \"lower–latin\" list style.",
"Upper-latin": "The tooltip text of the button that toggles the \"upper–latin\" list style."
}
2 changes: 2 additions & 0 deletions packages/ckeditor5-list/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@
"@ckeditor/ckeditor5-basic-styles": "^21.0.0",
"@ckeditor/ckeditor5-block-quote": "^21.0.0",
"@ckeditor/ckeditor5-clipboard": "^21.0.0",
"@ckeditor/ckeditor5-essentials": "^21.0.0",
"@ckeditor/ckeditor5-editor-classic": "^21.0.0",
"@ckeditor/ckeditor5-enter": "^21.0.0",
"@ckeditor/ckeditor5-font": "^21.0.0",
"@ckeditor/ckeditor5-heading": "^21.0.0",
"@ckeditor/ckeditor5-highlight": "^21.0.0",
"@ckeditor/ckeditor5-indent": "^21.0.0",
"@ckeditor/ckeditor5-link": "^21.0.0",
"@ckeditor/ckeditor5-remove-format": "^21.0.0",
"@ckeditor/ckeditor5-table": "^21.0.0",
"@ckeditor/ckeditor5-typing": "^21.0.0",
"@ckeditor/ckeditor5-undo": "^21.0.0"
Expand Down
6 changes: 6 additions & 0 deletions packages/ckeditor5-list/src/converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,12 @@ export function modelChangePostFixer( model, writer ) {
applied = true;
}

if ( item.hasAttribute( 'listStyle' ) ) {
jodator marked this conversation as resolved.
Show resolved Hide resolved
writer.removeAttribute( 'listStyle', item );

applied = true;
}

for ( const innerItem of Array.from( model.createRangeIn( item ) ).filter( e => e.item.is( 'element', 'listItem' ) ) ) {
_addListToFix( innerItem.previousPosition );
}
Expand Down
11 changes: 11 additions & 0 deletions packages/ckeditor5-list/src/indentcommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export default class IndentCommand extends Command {
* Indents or outdents (depending on the {@link #constructor}'s `indentDirection` parameter) selected list items.
*
* @fires execute
* @fires executeCleanup
*/
execute() {
const model = this.editor.model;
Expand Down Expand Up @@ -90,6 +91,16 @@ export default class IndentCommand extends Command {
writer.setAttribute( 'listIndent', indent, item );
}
}

/**
* Event fired by the {@link #execute} method.
*
* It allows to execute an action after executing the {@link ~IndentCommand#execute} method, e.g. adjusting
* attributes of changed list items.
*
* @event executeCleanup
*/
this.fire( 'executeCleanup', itemsToChange );
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a new pattern for already existing way of making such cleanups. So I'd say no for this (I might be missing a broader context though).

Ideally, we should make such cleanups using model post-fixer (basically the code that is run in listeners to this event). The itemsToChange can be obtained from editor.model.document.differ.getChanges().

I've used this:

function fixListAfterIndentListCommand( editor ) {
	return ( evt, changedItems ) => {
		let valueToSet;

		const changes = [ ...editor.model.document.differ.getChanges() ];

		for ( const change of changes ) {
			console.log( change.type, change.attributeKey );
		}

		const areSame = changedItems.length === changes.length;
		console.log( `Event ${ changedItems.length } Differ: ${ changes.length } \t => \tsame? ${ areSame }` );

to check if number of events will be the same as the number of elements touched by the commands. From this perspective it looks like you can obtain changed items from differ without adding an event.

ps.: The number of integration tests might be too small for this (judging just by the number of logged entries when running all list tests):

'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'
'attribute', 'listIndent'
'attribute', 'listIndent'
'Event 2 Differ: 2 	 => 	same? true'
'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'
'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'
'attribute', 'listIndent'
'attribute', 'listIndent'
'Event 2 Differ: 2 	 => 	same? true'
'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'
'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'
'attribute', 'listIndent'
'Event 1 Differ: 1 	 => 	same? true'

Process finished with exit code 0

Maybe one test with deeper nesting and/or bigger tree would be nice. So more than 2 items would change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I marked the event as private and added _ as prefix. (0143084)
But you're right. Post-fixer could be better solution here.

} );
}

Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-list/src/listcommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ export default class ListCommand extends Command {
writer.setAttribute( 'listType', this.type, element );
}
}

/**
* Event fired by the {@link #execute} method.
*
* It allows to execute an action after executing the {@link ~ListCommand#execute} method, e.g. adjusting
* attributes of changed blocks.
*
* @event executeCleanup
*/
this.fire( 'executeCleanup', blocks );
} );
}

Expand Down
36 changes: 36 additions & 0 deletions packages/ckeditor5-list/src/liststyles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module list/liststyles
*/

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import ListStylesEditing from './liststylesediting';
import ListStylesUI from './liststylesui';

/**
* The list styles feature.
*
* This is a "glue" plugin that loads the {@link module:list/liststylesediting~ListStylesEditing list styles editing feature}
* and the {@link module:list/liststylesolistui~ListStylesUI list styles UI feature}.
pomek marked this conversation as resolved.
Show resolved Hide resolved
*
* @extends module:core/plugin~Plugin
*/
export default class ListStyles extends Plugin {
pomek marked this conversation as resolved.
Show resolved Hide resolved
/**
* @inheritDoc
*/
static get requires() {
return [ ListStylesEditing, ListStylesUI ];
}

/**
* @inheritDoc
*/
static get pluginName() {
return 'ListStyles';
}
}
206 changes: 206 additions & 0 deletions packages/ckeditor5-list/src/liststylescommand.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
/**
* @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module list/liststylescommand
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import TreeWalker from '@ckeditor/ckeditor5-engine/src/model/treewalker';

/**
* The list style command. It is used by the {@link module:list/liststyles~ListStyles list styles feature}.
*
* @extends module:core/command~Command
*/
export default class ListStylesCommand extends Command {
/**
* Creates an instance of the command.
*
* @param {module:core/editor/editor~Editor} editor The editor instance.
* @param {String} defaultType The list type that will be used by default if the value was not specified during
* the command execution.
*/
constructor( editor, defaultType ) {
super( editor );

/**
* The default type of the list style.
*
* @private
* @member {String}
*/
this._defaultType = defaultType;
}

/**
* @inheritDoc
*/
refresh() {
this.value = this._getValue();
this.isEnabled = this._checkEnabled();
}

/**
* Executes the command.
*
* @param {Object} options
* @param {String|null} options.type The type of the list styles, e.g. 'disc' or 'square'. If `null` specified, the default
* style will be applied.
* @protected
*/
execute( options = {} ) {
const model = this.editor.model;
const document = model.document;
const position = findPositionInsideList( document.selection );
Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause "single item changed on range selection" issues.

From top of my head:

  1. Get elements/blocks from selection (to would get all selected list items).
  2. Use selection.getFirst/LastPostion() to get backward/forward siblings.


if ( !position ) {
return;
}

const listItems = [
...getSiblingNodes( position, 'backward' ),
...getSiblingNodes( position, 'forward' )
];

model.change( writer => {
for ( const item of listItems ) {
writer.setAttribute( 'listStyle', options.type || 'default', item );
}
} );
}

/**
* Checks the command's {@link #value}.
*
* @private
* @returns {String|null} The current value.
*/
_getValue() {
const listItem = this.editor.model.document.selection.getFirstPosition().parent;

if ( listItem && listItem.is( 'element', 'listItem' ) ) {
return listItem.getAttribute( 'listStyle' );
}

return null;
}

/**
* Checks whether the command can be enabled in the current context.
*
* @private
* @returns {Boolean} Whether the command should be enabled.
*/
_checkEnabled() {
const editor = this.editor;

const numberedList = editor.commands.get( 'numberedList' );
const bulletedList = editor.commands.get( 'bulletedList' );

return numberedList.isEnabled || bulletedList.isEnabled;
}
}

// Returns a position that is hooked in the `listItem` element.
// It can be the selection starting position or end.
//
// @param {module:engine/model/selection~Selection} selection
// @returns {module:engine/model/position~Position|null}
function findPositionInsideList( selection ) {
const startPosition = selection.getFirstPosition();

if ( startPosition.parent.is( 'element', 'listItem' ) ) {
return startPosition;
}

const lastPosition = selection.getLastPosition();

if ( lastPosition.parent.is( 'element', 'listItem' ) ) {
return lastPosition;
}

return null;
}

// Returns an array with all `listItem` elements that represents the same list.
//
// It means that values for `listIndent`, `listType`, and `listStyle` for all items
// are equal.
//
// @param {module:engine/model/position~Position} position Starting position.
// @param {'forward'|'backward'} direction Walking direction.
// @returns {Array.<module:engine/model/element~Element>
function getSiblingNodes( position, direction ) {
const items = [];
const listItem = position.parent;
const walkerOptions = {
ignoreElementEnd: true,
startPosition: position,
shallow: true,
direction
};
const limitIndent = listItem.getAttribute( 'listIndent' );
const nodes = [ ...new TreeWalker( walkerOptions ) ]
.filter( value => value.item.is( 'element' ) )
.map( value => value.item );

for ( const element of nodes ) {
// If found something else than `listItem`, we're out of the list scope.
if ( !element.is( 'element', 'listItem' ) ) {
break;
}

// If current parsed item has lower indent that element that the element that was a starting point,
// it means we left a nested list. Abort searching items.
//
// ■ List item 1. [listIndent=0]
// ○ List item 2.[] [listIndent=1], limitIndent = 1,
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
jodator marked this conversation as resolved.
Show resolved Hide resolved
//
// Abort searching when leave nested list.
if ( element.getAttribute( 'listIndent' ) < limitIndent ) {
break;
}

// ■ List item 1.[] [listIndent=0] limitIndent = 0,
// ○ List item 2. [listIndent=1]
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
//
// Ignore nested lists.
if ( element.getAttribute( 'listIndent' ) > limitIndent ) {
continue;
}

// ■ List item 1.[] [listType=bulleted]
// 1. List item 2. [listType=numbered]
// 2.List item 3. [listType=numbered]
//
// Abort searching when found a different kind of a list.
if ( element.getAttribute( 'listType' ) !== listItem.getAttribute( 'listType' ) ) {
break;
}

// ■ List item 1.[] [listType=bulleted]
// ■ List item 2. [listType=bulleted]
// ○ List item 3. [listType=bulleted]
// ○ List item 4. [listType=bulleted]
//
// Abort searching when found a different list style.
if ( element.getAttribute( 'listStyle' ) !== listItem.getAttribute( 'listStyle' ) ) {
break;
}

if ( direction === 'backward' ) {
items.unshift( element );
} else {
items.push( element );
}
}

return items;
}
Loading