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

The InsertTableCommand should break paragraph where the selection is anchored #3181

Closed
jodator opened this issue May 30, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-table#111
Assignees
Labels
package:table type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@jodator
Copy link
Contributor

jodator commented May 30, 2018

As @oleq pointed out:

Why is the table inserted after current block instead of splitting it (see v4 https://ckeditor.com/ckeditor-4/)? It's not very intuitive IMO.

kapture 2018-05-30 at 13 29 45

@oleq oleq changed the title The InsertTableCommand should break paragraph where the selection is The InsertTableCommand should break paragraph where the selection is anchored May 30, 2018
@Reinmar
Copy link
Member

Reinmar commented May 30, 2018

That's not critical so I'm moving this out of iteration 18.

And as for expected result, please use model.insertContent() which will take care of handling situations like inserting this into an empty paragraph (should replace it) or into a block quote (should split p and bQ).

@Reinmar
Copy link
Member

Reinmar commented May 30, 2018

BTW, the uploadImage button does something similar now – it also finds the "optimal insertion position". This behaviour makes sense especially for the block toolbar (Letters-like implementation). So, I'd say that the table feature should work in the same way.

cc @oskarwrobel @pjasiun

@pjasiun
Copy link

pjasiun commented Jun 11, 2018

Yep, we had a long discussion if an image insertion should break a block and agreed it should not, see https://github.com/ckeditor/ckeditor5-image/blob/076591e7f3b2eda73d172d6dcbabf8de3972aeba/src/imageupload/utils.js#L24-L38

However, image and tables are not exactly the same case. For images, we had mostly image drop in mind. It is hard to select position precisely when you drop, what was an argument to prevent block split. An image can be also aligned to the side, to be an illustration of the paragraph. To do so, the image needs to be inserted before the block. This is why the image is usually inserted before the block (except the case when the selection is at the end of the block).

In case of tables, I am not sure if they should break blocks. What I am sure, is that the table should be inserted before the block if the selection is at the beginning of the block. Otherwise, inserting tables using block toolbar will bring bad UX.

@pjasiun
Copy link

pjasiun commented Sep 10, 2018

insert-before-after

We added tables feature to Letters and now it is inconsistent that image is inserted before and table after the paragraph.

@pjasiun
Copy link

pjasiun commented Sep 10, 2018

It is exceptionally bad when I insert into an empty paragraph: table feature leaves an empty paragraph instead of replacing it:

insert-empty

@jodator jodator self-assigned this Sep 10, 2018
@Reinmar
Copy link
Member

Reinmar commented Sep 11, 2018

Just so you don't need to worry: model.insertContent() takes care of inserting into an empty paragraph.

We added tables feature to Letters and now it is inconsistent that image is inserted before and table after the paragraph.

And for that we need to expose the findOptimalInsertionPosition() from image upload utils. I wonder... where should it end? Maybe it could become a part of model.insertContent() itself? An option maybe?

@Reinmar
Copy link
Member

Reinmar commented Sep 11, 2018

But let's continue this discussion in #1243.

pjasiun referenced this issue in ckeditor/ckeditor5-table Sep 18, 2018
Other: Table feature should insert table the same way as other widgets. Closes #27.

BREAKING CHANGE: The `TableUtils#createTable()` method now accepts model `Writer` instance instead of `Position`. The method no longer inserts created table to the model - use returned value instead.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:table labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants