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

Implement table commands #3166

Closed
jodator opened this issue Apr 16, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-table#10
Closed

Implement table commands #3166

jodator opened this issue Apr 16, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-table#10
Assignees
Labels
package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@jodator
Copy link
Contributor

jodator commented Apr 16, 2018

The commands that need to be implmemented:

  • insert table (done)
  • insert row (done)
  • insert column (done but requires check as something looks broken)
  • remove row
  • remove column
  • split cell
  • change the table header rows/columns
  • merge cells (might be moved to milestone 2 as UI is also needed)
@jodator jodator self-assigned this Apr 16, 2018
@Reinmar
Copy link
Member

Reinmar commented Apr 17, 2018

Note that a cell can be split vertically or horizontally. And that it only makes sense if there's "merge cells" too.

@jodator
Copy link
Contributor Author

jodator commented Apr 25, 2018

As I have almost everything working more or less I'd like to precise some implementation details.

  1. insert table - rather clear. inserts rows x columns sized table

  2. insert row - inserts a given number of rows into currently selected table at passed index of currently selected table. Inconsistent usage - passes an index.

editor.execute( 'insertRow', { rows: 7, at: 4 } );
  1. insert column - similary to insert row:
editor.execute( 'insertColumn', { columns: 3, at: 0 );
  1. remove row - removes row of currently selected table cell. Only one row at a time.
editor.execute( 'removeRow' );
  1. remove column - removes a column of currently selected table cell. Only column row at a time.
editor.execute( 'removeColumn' );
  1. split cell - as a started I've implemented this as unmerge cell which simply splits cells on given rowspan x colspan cells. As this does not work as in CKEditor 4 I will update its usage to match CKEditor 4's but the question is: Do we want/need unmerge cell command as it is already implemented?

  2. set table headers - sets the new headers for table. Possible change: do not change rows/columns if not passed directly - now it will set to 0 if not passed.

editor.execute( 'setTableHeaders', { rows: 2 } ); 
editor.execute( 'setTableHeaders', { rows: 0, columns: 3 } );
  1. merge cells - it is currently implemented as set of editor commands: (mergeLeft, mergeRight, mergeUp, mergeDown). Each command's isEnabled is true whenever currently selected cell (selection focus) can be merged with one on the given direction (ie on the left). It can only be merged if its rowspan (or colspan for up/down) is the same as currently selected one). For caching purposed command's value is set to a mergeable cell.
editor.execute( 'mergeLeft' );
editor.execute( 'mergeUp' );

/cc @Reinmar, @oleq (as those will work with UI).

@jodator
Copy link
Contributor Author

jodator commented Apr 25, 2018

ps.: the command names are rather secondary thing for now - I'd like to see your opinions on behavior and/or usage in the first place.

@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2018

I think that our primary objective should be to make all commands work as you implemented mergeLeft/Right/Up/Down, removeRow, etc. They reflect the editing operations which you'll do with the UI. They can then also have a state because they are applied to the current selection, not to a place specified as their params.

You implemented the insertRow and insertColumn commands like utility functions. We need those utils somewhere (e.g. editor.plugins.get( 'TableUtils' ).insertRow( { at: 1 } )), but commands are more than this. They need to make implementing the UI a no-brainer. That's why we require from them that they have a state too and that they work in the selection's location and take care of the selection.

So, we need commands like:

  • insertRowAbove – inserts row above a selected cell; disabled if the selection is not in a table
  • insertRowBelow
  • insertColumnRight
  • insertColumnLeft
  • mergeCellRight – disabled if there's no cell next to it or if not in a table
  • mergeCellLeft
  • mergeCellUp
  • mergeCellDown
  • setTableRows – also disabled if not in a table

BTW. I have a problem with these command names... What if we'll have something else which can be merged right or has rows? I considered prefixing all commands with "table" but I guess we can give the table feature a priority. We'll prefix the 2nd+ commands :D

PS. Mind that I added "Cell" to merge* cmnds to avoid too generic names.

@jodator
Copy link
Contributor Author

jodator commented Apr 30, 2018

@Reinmar Thank's - that's why I've posted this - to be sure which way is better. I'll update current implementation of the commands.

@scofalik
Copy link
Contributor

scofalik commented May 15, 2018

IMO those mergeCell commands are temporal. I think that the final solution should be just mergeCells and would merge all cells within the selection.

EDIT: Same with cell splitting - personally, for me, it would be natural and sufficient if after splitting cell it would just split "totally". I mean that the merged cell would split into the smallest cells. It wouldn't be a problem if then I'd be able to select some cells and then merge it again.

@jodator
Copy link
Contributor Author

jodator commented May 15, 2018

@scofalik probably but cell merging from the selection is phase 2 so this solution will last at least for some time.

@scofalik
Copy link
Contributor

scofalik commented May 15, 2018

Of course, I realize that. BTW. I just edited the previous post.

Reinmar referenced this issue in ckeditor/ckeditor5-table May 29, 2018
Feature: Initial table support. Closes #4. Closes #7. Closes #9.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). 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:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants