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

I/6521: Add check for heading columns in merge cells command. #306

Merged
merged 11 commits into from
Apr 22, 2020
Merged

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Apr 20, 2020

Suggested merge commit message (convention)

Other: Merge cells command should not merge cells from heading columns with other cells. Closes ckeditor/ckeditor5#6521.


@coveralls
Copy link

coveralls commented Apr 20, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling fa958a1 on i/6521 into 0e25d38 on master.

@jodator
Copy link
Contributor Author

jodator commented Apr 20, 2020

I've found an undo bug: ckeditor/ckeditor5#6634. Also on master so a separate ticket.

Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

I would also add test like this:

it( 'should be false if selection has cells only from column headers and other cells - ???', () => {
    setData( model, modelTable( [
        [ '00[]', '01', '02', '03' ],
        [ '10', '11', '12', '13' ]
    ], { headingColumns: 2, headingRows: 1 } ) );

    tableSelection._setCellSelection(
        root.getNodeByPath( [ 0, 0, 0 ] ),
        root.getNodeByPath( [ 0, 1, 2 ] )
    );

    expect( command.isEnabled ).to.be.false;
} );

src/commands/mergecellscommand.js Outdated Show resolved Hide resolved
}

const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
const columnIndexes = getColumnIndexes( tableCells );
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is not the scope of this PR but getColumnIndexes confused me a little, maybe it should be sth like getFirstLastColumnIndexes? Same for getRowIndexes -> getFirstLastRowIndexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that this is not the scope of this PR

Yep, for me, it is outside a PR's scope. I'm fine with opening a new PR/issue for this. But here I didn't touch that code.

jodator and others added 3 commits April 22, 2020 07:20
@jodator jodator requested a review from niegowski April 22, 2020 05:29
@jodator
Copy link
Contributor Author

jodator commented Apr 22, 2020

I would also add test like this:

Added this in 6c3ad1d  kinda redundant but why not.

src/commands/mergecellscommand.js Outdated Show resolved Hide resolved
src/commands/mergecellscommand.js Outdated Show resolved Hide resolved
@oleq oleq merged commit 856889e into master Apr 22, 2020
@oleq oleq deleted the i/6521 branch April 22, 2020 15: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.

Merge cells command should not merge cells from column header with other cells
4 participants