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

Improvements for integration with track changes #202

Merged
merged 11 commits into from
Aug 14, 2019
Merged

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Aug 13, 2019

Suggested merge commit message (convention)

Feature: TableWalker will now return cell value also for spanned cells when traversing a table with includeSpanned option set to true. Additionally, isSpanned property was introduced in returned values.
Internal: Improvements in table plugin allowing for integration with track changes.

BREAKING CHANGE: TableWalker will not return undefined as cell value for spanned cells anymore. Use isSpanned instead.


Additional information

Requires ckeditor/ckeditor5-engine#1781

@scofalik scofalik requested a review from pjasiun August 13, 2019 11:15
@pjasiun pjasiun requested review from jodator and removed request for pjasiun August 13, 2019 11:27
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

I'm not happy with the change of yielding spanned cells. We loose the information when using includeSpanned: true that currently yielded value is spanned by other cell.

So the idea before inlcudeSpanned: true was to yield every table cell location (not a table cell). So if table should be 3x3 cells and have some cells merged (spanned over others) then it would have for instance 6 child table cells instead of 9.

Iterating over table in normal mode would yield 6 values while using includeSpanned: true should yield all 9 locations from which 3 would be not have a table cell but will be spanned by others.

It's not ideal API and probably confusing one. We can rename the options to make it clearer. Also I would keep the information about the "spanned" cell. Previously it was cell: undefiend but we can make this explicit as isSpanned: true or cell: undefined, spannedBy: cell. There is a possibility that the logic of other command would simplify by that.

src/commands/setheadercolumncommand.js Show resolved Hide resolved
src/commands/setheadercolumncommand.js Outdated Show resolved Hide resolved
src/commands/setheaderrowcommand.js Show resolved Hide resolved
src/commands/setheaderrowcommand.js Outdated Show resolved Hide resolved
src/tableutils.js Show resolved Hide resolved
src/tablewalker.js Outdated Show resolved Hide resolved
tests/tablewalker.js Show resolved Hide resolved
@scofalik
Copy link
Contributor Author

As far as includeSpanned and cell value. (I think) I understand the intention behind includeSpanned and it worked fine but in my case, I needed to get to the cell that is spanning over given table location. For example, when I was iterating over all locations in a column, I wanted to get both cells that "starts" in that column (those were added to a suggestion for sure) but also I wanted to store spanned cells because they might need to be added to a suggestion too (upon further processing).

I've checked if changing cell from undefined breaks anything and it didn't (so no scripts depended on undefined value). So, for me, not passing a value there is wasting an information that we have for free. I am fine for adding isSpanned though, for theoretical future needs.

@jodator
Copy link
Contributor

jodator commented Aug 14, 2019

I am fine for adding isSpanned though, for theoretical future needs.

So let's do this - we shouldn't loose that information. Also update the commit message regarding this change.

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Just one question we're OK.

@scofalik
Copy link
Contributor Author

Okay, I updated docs and fixed what we agreed on fixing.

src/tablewalker.js Outdated Show resolved Hide resolved
@jodator
Copy link
Contributor

jodator commented Aug 14, 2019

Waits for: ckeditor/ckeditor5-engine#1781.

@jodator jodator merged commit 07e8736 into master Aug 14, 2019
@jodator jodator deleted the cf/tc-tables branch August 14, 2019 12:06
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.

2 participants