I/6049: Introduced the table cell properties UI

[oleq]

Suggested merge commit message (convention)

Feature: Introduced the table cell properties UI. Closes ckeditor/ckeditor5#6049.

---

Additional information

Requirements

• The CI constellation branch in https://github.com/ckeditor/ckeditor5/tree/i/6049.
• This PR lives on top of I/3287: Table and table cell properties plugins #224.

Dependencies:

• (ckeditor5-alignment) Made the alignment icons part of ckeditor5-core.
• (ckeditor5-core) Added vertical alignment icons and moved horizontal alignment icons from ckeditor5-alignment
• (ckeditor5-ui) Created the LabeledView and tweaked the DropdownView and LabelView
• (theme) Added styles for the LabeledView

Round up

New things in the PR:

• TableCellPropertiesUI (controller plugin)
• TableCellPropertiesView (form view)
• (private) FormRowView a class shared between the TableCellPropertiesView and the (future) TablePropertiesView. Initially, I wanted to make it part of ckeditor5-ui then I figured we may need something better like a form builder so making it a public API does not make sense ATM (until we do more than 2 table forms).
• 2 new icons for the cell properties view and (future) table properties view.

Followups:

• Table properties form UI,
• Rich color picker,
• Property values' validators,
• Dooooooocs

Also:

• RTL support,
• Border style preview,
• Share form header with special characters dropdown,
• Deprecate LabeledInputView in favor of LabeledView
• Declarative form builder,
• Enabling/disabling property features (granular approach)

[jodator]

I've made a birds-eye review. Most of them don't require action in this PR as the code as general is really nice :)

Edit: aaaaand I've just read the follow-ups and I can see the declarative from builder which probably address the coment about form view class :)

[jodator]

I wonder if it wouldn't be better to separate logic related to each value of CELL_PROPERTIES as here and in other places there are loops and checks.

This will have some code duplication for sure. But maybe it will be more clear or will require fewer changes if the logic for one of the values would change. Another consideration is enabling/disabling (by configuration - not in runtime) some properties from editing.

then some code would be

handleProperty() {
    this.listenTo( view, 'change:property', ... );
    // some other listener for the form view setting
}

[jodator]

❤️ 📜 I just love those wall of text in this whole feature :D

[jodator]

Just a thought - awfully long and mixes logic for different properties. I think that it might be a good candidate for a follow up / decision to make for the future. It's OK for the MVP :)

[jodator]

ps.: Also the above comment can have little sense if we can't have a need for enabling or disabling some properties.

[jodator]

I think that some of the intermediate values could be dropped in favor of a shorter method.

[jodator]

I've wrapped the test in either describe( 'table cell properties', () => {} ) or describe( 'table properties', () => {} ) to group those tests. This is just cosmetic but it helped me to group those tests in a report:

vs current:

and the same in CLI:

ps.: I'll hijack this comment for standup tension as current table tests are a mes :D

[oleq]

Self.R-: Use setTableCellWithObjectAttributes in tests.

[oleq]

Self.R-: Use setTableCellWithObjectAttributes in tests.

Nah, checked it and it's not necessary after all.

[coveralls]

Coverage remained the same at 100.0% when pulling 10ab4ae on i/6049 into d905bef on master.

[jodator]

Everything looks and works very good :) I have only one concern about the main UI class an how the bindings are constructed there. However, we can move that part to a follow up to unblock other work but we should decide what to do with this for the table properties UI.

[jodator]

LGTM 👍