Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Integrated multi cell selection with set column /row header commands #268

Merged
merged 12 commits into from
Mar 11, 2020

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Mar 10, 2020

Suggested merge commit message (convention)

Type: Integrated multi cell selection with set column /row header commands. Closes ckeditor/ckeditor5#6127.


Additional information

After this PR the header command state could get a third state - "mixed" because it's possible to make selection where only part of it contains headers:

However to simplify things, I implemented it in a way that command state is true only when all selected cells contain header. Otherwise it's false.

I have based this PR on #266 as this PR adds the API that was useful for this enhancement. Rebased onto master, after #265 added API that I was mocking before.

@coveralls
Copy link

coveralls commented Mar 10, 2020

Coverage Status

Coverage increased (+0.2%) to 97.138% when pulling 7bc9a05 on i/6127 into c088814 on master.

@mlewand
Copy link
Contributor Author

mlewand commented Mar 10, 2020

Rebased onto the latest master branch from after #265. @ckeditor/qa-team please give it a testing.

@Mgsy Mgsy requested review from Mgsy and FilipTokarski March 10, 2020 13:06
@mlewand
Copy link
Contributor Author

mlewand commented Mar 10, 2020

Just pushed one missing commit, sorry for that 🙈 @ckeditor/qa-team

~ Conflicts:
~	src/commands/setheadercolumncommand.js
~	src/commands/setheaderrowcommand.js
@mlewand
Copy link
Contributor Author

mlewand commented Mar 11, 2020

Merged latest master, resolved conflicts.

@FilipTokarski
Copy link
Member

FilipTokarski commented Mar 11, 2020

I'm not sure what is the desired behaviour here, but this cell splitting looks a bit weird:

Safari

  • ( please note how on Safari it changes whole selection, it stays on one cell in column d, under the balloon )
    table_header_safari1

Chrome:

table_header_chrome1

EDIT:

  • if you do similar multi-cell selections on multi-merged cells, but use header column instead, cells are not splitting

Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@FilipTokarski FilipTokarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from what I mentioned above, LGTM.

@mlewand
Copy link
Contributor Author

mlewand commented Mar 11, 2020

I'm not sure what is the desired behaviour here, but this cell splitting looks a bit weird:

I believe that exactly the same behavior is present on master branch, right? Neither was I a fan of this, but here I focused on bringing support for multi select, not that much on fixing underlying table plugin logic.

~ Conflicts:
~	src/commands/setheadercolumncommand.js
@mlewand
Copy link
Contributor Author

mlewand commented Mar 11, 2020

Merged the master branch and resolved conflicts.

@Reinmar Reinmar self-assigned this Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table selection handling: Set heading rows, column commands
5 participants