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

Should it be possible to create two sublists in one item? #2971

Closed
Reinmar opened this issue Mar 20, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-list#42
Closed

Should it be possible to create two sublists in one item? #2971

Reinmar opened this issue Mar 20, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-list#42
Assignees
Labels
package:list type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 20, 2017

It's now possible to create such content:

image

<ul>
  <li>
    abc
    <ul><li>abc</li></ul>
    <ol><li>abc</li></ol>
  </li>
  <li>abc</li>
</ul>

It's also possible to create such content in CKEditor 4. The process is the same way – just create a sub list with two items and convert one of them to a different type.

Fun fact – it makes CKEditor 4 very unstable (e.g. backspacing around these list items), so I guess it just wasn't anticipated.

CKEditor 5 is actually handling this quite well, but I'm not sure that this is the kind of a list that you should be able to achieve.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 20, 2017

Two solutions:

  • Changing the type of an item should reset indent to 0 and fix the following indents.
  • Changing one item's type should change the type of the entire sublist.

I'm more for the 2nd solution.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 21, 2017

Note: Whichever way we go, I think that we'll need to have a post-fixer or something because there may be more ways to create such a lists than by changing the type of one of the items.

@scofalik
Copy link
Contributor

scofalik commented Mar 21, 2017

This is a feature :(. I put a lot of effort to enable this.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 21, 2017

This is a feature :(. I put a lot of effort to enable this.

I thought so. But it really doesn't seem to be necessary. At least to me. cc @fredck and @oleq and @Comandeer.

@oleq
Copy link
Member

oleq commented Mar 21, 2017

Even if possible, I think that two subsequent lists (of any type) in a nested item make little sense. Nested items are to visualize the hierarchy, and the case that you presented isn't one.

You can create one list after another at the 0-level (top paragraph-level) because they don't share any root, they can visualize some different data. But such structure in a nested item feels like a nonsense to me.

@fredck
Copy link
Contributor

fredck commented Mar 21, 2017

Sorry to say this, @scofalik, but this feature sounds like useless/pointless. I'm all for changing it, especially if this means cleaning up our code :)

@Reinmar
Copy link
Member Author

Reinmar commented Mar 21, 2017

Waiting for a PR removing 1000LOC :D

@scofalik
Copy link
Contributor

I'm glad you like the feature . . .

@scofalik scofalik self-assigned this Mar 22, 2017
@scofalik
Copy link
Contributor

scofalik commented Mar 22, 2017

Changing one item's type should change the type of the entire sublist.

I think that quoted solution is better, since it is:

  • easier to implement,
  • makes more sense from users point of view (by changing list type, the user for sure do not want to break the whole list into pieces),
  • post-fixer is easier (whenever you insert/move-in list item just check surrounding items' type and change inserted item's type if needed).

I'll try to go this way.

Anyway, fun fact: I said that it I did it on purpose like this and I put some work into it. The thing is that the way converters work now is very convenient for this "feature". Mostly what remove/change indent/change type converters do is:

  • break the list before and after changed/removed list item,
  • remove the list item, or change parent's name from/to <ul>/<ol> (list item's parent contains only that list item as we broke before and after the item)
  • reinsert the list item with its parent (when changing indent -- however inserting item is a complicated algorithm that takes care of a lot of breaking and moving stuff in view).

I could easily force this solution without even touching converters much or at all. Post-fixer will change wrong type (and trigger conversion). ListCommand will take care of changing type of all sibling list items. I will try to go this way as "first solution" to unlock the PR and then will add followup to clean converters code (I am sure it will be possible to clean and simplify them, at least a bit).

EDIT: Removing needs to be post fixed with this solution - you could have a selection spanning over two different nested lists:

* -----
  * ---[--
  * ------
1 ------
 1.1 --]---
 1.2 -----

@Reinmar
Copy link
Member Author

Reinmar commented Mar 22, 2017

EDIT: Removing needs to be post fixed with this solution - you could have a selection spanning over two different nested lists:

I think that insertion needs to be post-fixed too – you shouldn't be able to paste ul in a middle of ol.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 22, 2017

EDIT: I've just seen https://github.com/ckeditor/ckeditor5-list/issues/48.

@scofalik
Copy link
Contributor

scofalik commented Mar 22, 2017

Oh, sorry, I thought that I wrote something earlier but I didn't :). ckeditor/ckeditor5-list#48 is unrelated.

I thought that I already wrote that insertion will have to be handled but I wrote only this:

Post-fixer will change wrong type (and trigger conversion)

Of course insert also have to be fixed (and move too).

Reinmar referenced this issue in ckeditor/ckeditor5-list Mar 24, 2017
Feature: Added support for nested lists.

These changes close a wide range of issues. Closes #8. Closes #9. Closes #30. Closes #36. Closes #37. Closes #38. Closes #39. Closes #40. Closes #41. Closes #44. Closes #45.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-list Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:list labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:list type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants