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

Support col[span] and pts in paste from excel #14720

Merged
merged 16 commits into from
Aug 14, 2023

Conversation

Dumluregn
Copy link
Contributor

@Dumluregn Dumluregn commented Aug 2, 2023

Suggested merge commit message (convention)

Fix (table,paste-from-office): Tables pasted from MS Excel will have proper column widths. Closes #14521. Closes #14516.


Additional information

See the detailed explanation what is going on here.

There's also another PR that should be merged after this one: https://github.com/cksource/ckeditor5-commercial/pull/5399.

@dufipl
Copy link
Contributor

dufipl commented Aug 3, 2023

This case is still not working: #14521 (comment)

@illia-stv
Copy link
Contributor

CI failing

@Dumluregn Dumluregn requested review from illia-stv and mlewand August 4, 2023 12:27
Copy link
Contributor

@illia-stv illia-stv left a comment

Choose a reason for hiding this comment

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

For me looks good✅

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Good solution, just few minor remarks and we're good to go.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge it once CI passes.

@mlewand mlewand merged commit cc319ca into master Aug 14, 2023
@mlewand mlewand deleted the ck/14521-support-cell-widths-in-paste-from-excel branch August 14, 2023 14:21

// Fill the array with 'auto' values if the number of columns is higher than number of declared values.
columnWidths = Array.from( { length: columnsCount }, ( _, index ) => columnWidths[ index ] || 'auto' );

if ( columnWidths.length != columnElements.length || columnWidths.includes( 'auto' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not triggering column width normalization for such case:

<table>
    <colgroup>
        <col style="width: 30pt;">
        <col style="width: 70pt;">
    </colgroup>
    <tbody>
        <tr>
            <td>a</td>
            <td>b</td>
        </tr>
    </tbody>
</table>

And the final table ends up like this:

<figure class="table">
    <table class="ck-table-resized">
        <colgroup>
            <col style="width:30%;">
            <col style="width:70%;">
        </colgroup>
        <tbody>
        <tr>
            <td>a</td>
            <td>b</td>
        </tr>
        </tbody>
    </table>
</figure>

so pt values are treated as they would be %, the table gets full page width and the original sizes are not preserved.

The column width is normalized in post-fixer (but should be normalized before post-fixers are triggered).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants