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

Jump to bottom

Introduce a common "get selected cells" util in src/utils.js #6358

Closed
Reinmar opened this issue Feb 28, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-table#265
Closed

Introduce a common "get selected cells" util in src/utils.js #6358

Reinmar opened this issue Feb 28, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-table#265
Assignees
@oleq
Labels
package:table type:improvement This issue reports a possible enhancement of an existing feature.
Milestone
iteration 30 (v18...

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 28, 2020

📝 Provide a description of the improvement

Right now we have:

  • getSelectedTableCells in tablecellpropertycommand.js,
  • getSelectedTableCells in tableselection.js (which is invalid as it's only reasonable to be used while the selection is being made – hence, it should be a private util).

If you'd like to see this improvement implemented, add a 👍 reaction to this post.
@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. status:confirmed package:table labels Feb 28, 2020
@Reinmar Reinmar added this to the iteration 30 milestone Feb 28, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Feb 28, 2020

  • getSelectedTableCells in tablecellpropertycommand.js,

The implementation of this function is naive, and the function to be introduced should not work like this. The aformentioned implementation returns the same cell multiple times. The table cell props form works only because the value is deduped later on (in getSingleValue()).

@Reinmar Reinmar mentioned this issue Feb 28, 2020
Merged
@Reinmar
Copy link
Member Author

Reinmar commented Mar 2, 2020

Related ticket: #6364.

@Reinmar Reinmar mentioned this issue Mar 2, 2020
Closed
@oleq
Copy link
Member

oleq commented Mar 5, 2020

What about https://github.com/ckeditor/ckeditor5-table/blob/7749fc82e0e3670d6817aff151ef45b8202682e7/src/tableselection/utils.js#L39-L52?

@Reinmar
Copy link
Member Author

Reinmar commented Mar 6, 2020

ckeditor/ckeditor5-table#263 introduced a new function (plugin method) for getting selected cells (from the model document selection).

@Reinmar Reinmar assigned oleq and unassigned mlewand Mar 6, 2020
@oleq oleq mentioned this issue Mar 9, 2020
Merged
mlewand added a commit to ckeditor/ckeditor5-table that referenced this issue Mar 10, 2020
@mlewand
Merge pull request #265 from ckeditor/i/6358
eb74673 
Internal: Implemented the `getSelectedTableCells()` helper based on `Range#getContainedElement()`. Code refactoring. Closes [ckeditor/ckeditor5#6358](ckeditor/ckeditor5#6358).
@oleq oleq mentioned this issue Mar 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oleq oleq

Labels
package:table type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Milestone
iteration 30 (v18.0.0)
Development

Successfully merging a pull request may close this issue.

i/6358: Implemented the getSelectedTableCells() helper ckeditor/ckeditor5-table
3 participants
@Reinmar @oleq @mlewand