List style feature #7861

pomek · 2020-08-18T05:23:54Z

Suggested merge commit message (convention)

Feature (list): Introduced the list styles feature that allows customizing the list's marker of the list item elements. Closes #7801.

Feature (theme-lark): Creates styles for the ListStylesUI plugin (see #7803).

MINOR BREAKING CHANGE (ui): It is now possible to override existing components when adding new ones to the component factory (previously an error was thrown) (see #7803).

Additional information

UI part of the feature was reviewed in #7845.
Docs: #7881 (can be merged after this PR).

… paragraph.

…ins.

…mponent factory.

Internal (list): Created the list styles UI. Closes #7803.

pomek · 2020-08-19T10:44:27Z

Keep in mind that I had some issues with the implementation and the feature could contain some bugs. See #7879.

pomek · 2020-08-19T10:45:49Z

@ckeditor/qa-team, could you check how critical are issues described in the #7879 issue? I mean, are we able to merge the feature core and fix bugs later?

pomek · 2020-08-19T10:59:01Z

packages/ckeditor5-list/src/liststylesui.js

+				editor.execute( parentCommandName );
+				editor.execute( 'listStyles', { type } );


Shouldn't we wrap the code in the model#change() block in order to have single undo step?

It might do the trick AFAIR. So for now might be OK.

The other thing that came to my mind might be that ListSyyle command might also do the parentCommand stuff but as per PR requirements to not-block this PR a wrap will be quicker.

Reinmar · 2020-08-19T11:02:30Z

One thing to keep in mind here is that the number of edge cases and weird cases and unclear cases will be significant. I'd heavily prioritize stability over correctness.

The other thing is that we're approaching the code freeze, so if the reviewer will stumble upon things that require minor code refactorings, those things should be split to a separate issue, to not block this PR.

Mgsy · 2020-08-19T11:54:50Z

Steps to reproduce

Open list styles manual test.
Select some text in a list item.
Press Ctrl + B

Current result

The editor crashes. It's interesting, as this manual test doesn't have Bold feature.

Error

liststylesediting.js:97 Uncaught TypeError: Cannot read property 'getStyle' of null
    at UpcastDispatcher.dispatcher.on.priority (liststylesediting.js:97)
    at UpcastDispatcher.fire (emittermixin.js:209)
    at UpcastDispatcher._convertItem (upcastdispatcher.js:249)
    at UpcastDispatcher.convert (upcastdispatcher.js:207)
    at Object.callback (datacontroller.js:414)
    at Model._runPendingChanges (model.js:830)
    at Model.change (model.js:175)
    at DataController.toModel (datacontroller.js:413)
    at MutationHandler._handleContainerChildrenMutations (injecttypingmutationshandling.js:109)
    at MutationHandler.handle (injecttypingmutationshandling.js:64)

Can't reproduce it after adding basic styles to the manual tests. Commit: 178ec10.

FilipTokarski · 2020-08-19T11:58:47Z

Steps:

Add two styled list items
Apply link on the second one
Select the second one and press Bulleted list on the toolbar

Expected: list changes to unordered

Actual: list changes to unordered only when you choose the specific style

Mgsy · 2020-08-19T12:14:27Z

Steps to reproduce

Create a new list with one nested item ( <ol><li>Test<ol><li>Test 2</li></ol></li></ol> ).
Select all items.
Add styling.

Current result

Only the root item is changed.

jodator

I've saw that there are some issues found so maybe checking if using differ instead of command's events might be relevant.

Abut naming - I've asked that question on the Slack channel as I'm not sure why the plural form and I might be lacking some conversation details.

If I commented something in the UI part - please ignore that. I've forgot instantly that this PR was partially reviewed. I would have some insights for retro though :D

I'll do the jsdocs/tests re-read later on.

To sum up and to not block:

The feature works, with minor bugs. Which, at least IMHO, we could pass.
The naming might be the most important one before releasing (PITA to do, but should be fairly fast).
crashes to be addressed/checked first.
The event thing (if no time left) should be at least hidden under @Private to have it refactor later as I'm almost 100% sure that we could do the cleanup without it.

packages/ckeditor5-list/src/converters.js

packages/ckeditor5-list/src/liststylesediting.js

jodator · 2020-08-19T12:53:42Z

packages/ckeditor5-list/src/indentcommand.js

+			 *
+			 * @event executeCleanup
+			 */
+			this.fire( 'executeCleanup', itemsToChange );


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.

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

packages/ckeditor5-list/src/liststyles.js

packages/ckeditor5-list/src/liststylescommand.js

packages/ckeditor5-list/src/liststylesediting.js

jodator · 2020-08-19T13:10:57Z

packages/ckeditor5-list/src/liststylesui.js

+				editor.execute( parentCommandName );
+				editor.execute( 'listStyles', { type } );


It might do the trick AFAIR. So for now might be OK.

The other thing that came to my mind might be that ListSyyle command might also do the parentCommand stuff but as per PR requirements to not-block this PR a wrap will be quicker.

packages/ckeditor5-ui/src/componentfactory.js

jodator · 2020-08-19T13:32:01Z

packages/ckeditor5-list/src/liststylescommand.js

+	execute( options = {} ) {
+		const model = this.editor.model;
+		const document = model.document;
+		const position = findPositionInsideList( document.selection );


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

From top of my head:

Get elements/blocks from selection (to would get all selected list items).

Use selection.getFirst/LastPostion() to get backward/forward siblings.

jodator · 2020-08-19T13:49:16Z

So I've also found similar issues to the above:

The 2-commands action behaves strangely because the first command (list type) changes all items to the other type and the second command (list style) applies style only to the top-most item.

So I think that my comment regarding how this command should work is valid (take elements from range or just the same ones as list type command does).

ps.: I've might found a bug in the original command as the one item does not change it's type 😮 .

jodator · 2020-08-19T14:02:40Z

ps.: I've might found a bug in the original command as the one item does not change it's type open_mouth .

It's some kind of downcast rendering glitch. I can't reproduce it on fresh content.

… in a list.

…arked as private.

pomek · 2020-08-19T18:42:05Z

So as for now, we have three main issues:

the command changes the most outdented list and ignores nested ones (assuming that selection has some)
sometimes render fails (refreshing the same scenario helps)
_executeCleanup pattern is not correct but it could be done as a followup after merging the PR.

The naming might be the most important one before releasing (PITA to do, but should be fairly fast).

Now the feature is called ListStyle.

jodator · 2020-08-20T06:31:25Z

@pomek I've updated an unrelated test name as it was bugging me ;)

jodator · 2020-08-20T13:34:32Z

@Reinmar if @pomek doesn't need to add anything else here I think that this might go to the testing phase as well.

pomek · 2020-08-20T14:12:38Z

Let me add a few tests that you mentioned and I'll check whether ListStyleCommand has all use cases covered.

pomek · 2020-08-21T06:18:25Z

@jodator, I added missing stuff. It's ready for merge.

pomek and others added 24 commits August 17, 2020 13:20

Added manual tests for the list styles feature.

9b24335

An initial implementation for the list styles feature.

b6fa8b9

Tests init.

3591a8d

An initial implementation of ListStyleCommand.

f3df005

The "listStyle" attribute should be removed when renaming listItem to…

a40d46e

… paragraph.

The IndentCommand could return nodes that were modified.

5a3e307

Many improvements with integration List style feature with other plug…

34b0e5c

…ins.

Added tests for ListStylesCommand, added integrations with other plug…

8f5ff69

…ins.

Restored a fancy test.

b42f5c7

Minor improvements.

05837c9

Additional improvements.

31e34de

Allowed overriding existing components when adding new ones to the co…

bafb120

…mponent factory.

Added icons for list styles.

b889ae0

Added CSS styles for the list styles UI.

3f14175

Implemented the ListStylesUI plugin.

c0d4212

Tests: Added tests for the ListStylesUI class.

13c418b

Added tooltips to list style buttons.

4a3254b

Added translation entries to contexts.json.

ebf5438

Fixed a typo in the package.json file.

b8eeaeb

Removed invalid change.

3d35fbf

Removed invalid change.

f312825

Clean.

f8faee1

Merge pull request #7845 from ckeditor/i/7803-list-styles-ui

87beb07

Internal (list): Created the list styles UI. Closes #7803.

Removed "custom" UI from the list styles manual test.

d3b2f9a

pomek marked this pull request as ready for review August 19, 2020 10:44

pomek commented Aug 19, 2020

View reviewed changes

jodator self-assigned this Aug 19, 2020

This comment has been minimized.

Sign in to view

pomek mentioned this pull request Aug 19, 2020

Docs for the List styles #7881

Merged

jodator suggested changes Aug 19, 2020

View reviewed changes

pomek added 6 commits August 19, 2020 20:05

Renamed the feature.

d8cf02d

Renamed an internal function.

879d47d

The single undo step will be created while selection was not anchored…

311f3a2

… in a list.

Added basic styles to the manual test.

178ec10

Use default value obtained from the command.

eaed650

Renamed the "executeCleanup" to "_executeCleanup". Now the event is m…

0143084

…arked as private.

Update forward indent command suite name.

39f0604

Changed a way that items for updating are obtained.

ac39cef

jodator approved these changes Aug 20, 2020

View reviewed changes

pomek added 4 commits August 21, 2020 07:53

Merge branch 'master' into i/7801

373ad4c

Marked "_executeCleanup" event as protected.

466d244

Added a test for changes introduced in ac39cef.

c7e2355

Clean up.

7c93510

List style is not a format attribute.

b3d5eb0

jodator merged commit 137dd28 into master Aug 21, 2020

jodator deleted the i/7801 branch August 21, 2020 07:09

pomek mentioned this pull request Apr 10, 2024

The list properties toolbar is overriden by List plugin #16192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List style feature #7861

List style feature #7861

pomek commented Aug 18, 2020 •

edited

Loading

pomek commented Aug 19, 2020

pomek commented Aug 19, 2020

pomek Aug 19, 2020

jodator Aug 19, 2020

Reinmar commented Aug 19, 2020 •

edited

Loading

Mgsy commented Aug 19, 2020 •

edited by pomek

Loading

FilipTokarski commented Aug 19, 2020

Mgsy commented Aug 19, 2020

This comment has been minimized.

jodator left a comment

jodator Aug 19, 2020

pomek Aug 19, 2020

jodator Aug 19, 2020

jodator Aug 19, 2020

jodator commented Aug 19, 2020

jodator commented Aug 19, 2020

pomek commented Aug 19, 2020

jodator commented Aug 20, 2020

jodator commented Aug 20, 2020

pomek commented Aug 20, 2020

pomek commented Aug 21, 2020

		editor.execute( parentCommandName );
		editor.execute( 'listStyles', { type } );

List style feature #7861

List style feature #7861

Conversation

pomek commented Aug 18, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

pomek commented Aug 19, 2020

pomek commented Aug 19, 2020

pomek Aug 19, 2020

Choose a reason for hiding this comment

jodator Aug 19, 2020

Choose a reason for hiding this comment

Reinmar commented Aug 19, 2020 • edited Loading

Mgsy commented Aug 19, 2020 • edited by pomek Loading

Steps to reproduce

Current result

Error

FilipTokarski commented Aug 19, 2020

Steps:

Mgsy commented Aug 19, 2020

Steps to reproduce

Current result

This comment has been minimized.

jodator left a comment

Choose a reason for hiding this comment

jodator Aug 19, 2020

Choose a reason for hiding this comment

pomek Aug 19, 2020

Choose a reason for hiding this comment

jodator Aug 19, 2020

Choose a reason for hiding this comment

jodator Aug 19, 2020

Choose a reason for hiding this comment

jodator commented Aug 19, 2020

jodator commented Aug 19, 2020

pomek commented Aug 19, 2020

jodator commented Aug 20, 2020

jodator commented Aug 20, 2020

pomek commented Aug 20, 2020

pomek commented Aug 21, 2020

pomek commented Aug 18, 2020 •

edited

Loading

Reinmar commented Aug 19, 2020 •

edited

Loading

Mgsy commented Aug 19, 2020 •

edited by pomek

Loading