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

I/3287: Table and table cell properties plugins #224

Merged
merged 135 commits into from
Jan 24, 2020
Merged

I/3287: Table and table cell properties plugins #224

merged 135 commits into from
Jan 24, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Oct 28, 2019

Suggested merge commit message (convention)

Feature: Introduce table properites conversion. Closes ckeditor/ckeditor5#6048.


PR constallation

Build on ckeditor5/i3287 branch: Build Status

Edit:
the build fails because watchdog detects StylesConverter as "connected" https://github.com/ckeditor/ckeditor5-watchdog/blob/73844f37f90ea0bf0e0a756752c62b1ce86e334b/src/watchdog.js#L452 because of the StylesConverter singleton... it is fixed by making StyleConverter a non-enumerable property.

This PR requires changes:

Description

This is a draft PR to start review process of this behemoth change. This PR will need at least two reviewers and probably more then 2 passes.

At this stage I'd like to review the general idea behind table styles:

  • Changes in the engine that enables styles normalization (separate PR)
  • Table properties model, conversion, desired output.

Conversion (* - means any value but can be constrained by Styles normalizer):

  1. TableProperties handles:
    • borderWidth=*, borderColor=*, borderStyle=* (<table style="boder-top:1px solid blue;...">):
    • backgroundColor=* (<table style="background-color:some-color">)
    • width=* / heigh=* ( <table style="width:xx">)
    • alignment=left|right|center (<table style="margin-left:0,margin-right:auto"> - as in MDN, upcasts also <table aling="left|right|center">).
  2. TableCellProperties handles:
    • borderWidth=*, borderColor=*, borderStyle=* (<td style="boder-top:1px solid blue;...">)
    • backgroundColor=* (<td style="background-color:some-color">)
    • padding=* (<td style="padding:2em">)
    • verticalAlignment=* (<td style="vertical-align:top">) - should be fixed to
    • horizontalAlignment=left|fight|center|justify (<td style="text-align:left|right|center"justify">)
  3. TableColumnRowProperties handles:
    • heigth=* (<td style="height:*">)
    • width=* (<td style="widith:*">)

@jodator
Copy link
Contributor Author

jodator commented Jan 20, 2020

@Reinmar This PR implements the two main plugins:

  1. TableCellProperties - glues *Editing and *UI parts.
  2. TableProperties - glues *Editing and *UI parts.

From that I've implemented 16 commands:

For table properties:

  • tableBorderColor - manipulates a table's borderColor attribute
  • tableBorderStyle - manipulates a table's borderStyle attribute
  • tableBorderWidth - manipulates a table's borderWidth attribute
  • tableAlignment - manipulates a table's alignment attribute
  • tableWidth - manipulates a table's width attribute
  • tableHeight - manipulates a table's height attribute
  • tableBackgroundColor - manipulates a table's backgroundColor attribute

For table cell properties:

  • tableCellBorderStyle - manipulates a tableCell's borderStyle attribute
  • tableCellBorderColor - manipulates a tableCell's borderColor attribute
  • tableCellBorderWidth - manipulates a tableCell's borderWidth attribute
  • tableCellHorizontalAlignment - manipulates a tableCell's horizontalAlignment attribute
  • tableCellWidth - manipulates a tableCell's width attribute
  • tableCellHeight - manipulates a tableCell's height attribute
  • tableCellPadding - manipulates a tableCell's padding attribute
  • tableCellBackgroundColor - manipulates a tableCell's backgroundColor attribute
  • tableCellVerticalAlignment - manipulates a tableCell's verticalAlignment attribute

Please validate if the names are OK (especially if the attribute name is OK ie borderColor vs tableBorderColor). Eventual change would be 20 min tops, so thats not an issue ;)

Most of the commands code is done in base commands which at least in MVP do the same: get a table or cells from selection and apply or remove attribute. If command operates on top/right/bottom/left value object it will set command.value only if all four are the same.

As a simplification (which also works) I've made that command sets one string value in the model instead of top/right/bottom/left object. This can be changed in follow up.

@jodator jodator requested a review from oleq January 22, 2020 12:42
@jodator jodator assigned oleq and unassigned Reinmar Jan 22, 2020
@jodator jodator removed the request for review from Reinmar January 22, 2020 12:43
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

LGTM. I pushed documentation fixes straight to the branch because otherwise, GH shows me Unicorns when submitting.

src/commands/utils.js Show resolved Hide resolved
src/tableproperties.js Show resolved Hide resolved
src/tableproperties/tablepropertiesediting.js Show resolved Hide resolved
src/tableproperties/tablepropertiesediting.js Outdated Show resolved Hide resolved
src/tableproperties/tablepropertiesediting.js Outdated Show resolved Hide resolved
tests/_utils/utils.js Outdated Show resolved Hide resolved
tests/manual/tableproperties.md Show resolved Hide resolved
@jodator jodator requested a review from oleq January 23, 2020 16:06
@jodator
Copy link
Contributor Author

jodator commented Jan 23, 2020

@oleq done. Please note that I've added small addition to the docs in the engine so this PR is needed also: ckeditor/ckeditor5-engine#1816.

Edges are more like contours around the box (containing or not containing the padding, border, etc.) and the box has always 4 sides (top, left, bottom, right).
src/commands/utils.js Outdated Show resolved Hide resolved
src/commands/utils.js Outdated Show resolved Hide resolved
@oleq oleq changed the title I/3287: Table styles I/3287: Table and table cell properties plugins Jan 24, 2020
@oleq oleq merged commit d57085d into master Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants