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

I/6112: Implemented the table properties form#232

Merged
jodator merged 14 commits intomasterfrom
i/6112
Feb 3, 2020
Merged

I/6112: Implemented the table properties form#232
jodator merged 14 commits intomasterfrom
i/6112

Conversation

@oleq
Copy link
Copy Markdown
Member

@oleq oleq commented Jan 31, 2020

Suggested merge commit message (convention)

Feature: Implemented the table properties form. Closes ckeditor/ckeditor5#6112.


Additional information

CI and the constellation

Requires

  • ckeditor5-ui — the new property allowing to make the toolbar compact (for all alignment toolbars in both table and table cell properties forms)
  • ckeditor5-theme-lark:
    • styles for the compact toolbar,
    • styles fro the table properties form,
    • I found out that we didn't separate wireframe and theme styles in the previous PR so this PR does that
    • styles for private components like FormRowView and FormHeaderView.

Remarks

TablePropertiesUI and TableCellPropertiesUI are very similar (same with the views). At first, I wanted to create a base class (classes) for all of them (and/or a tool to help compose them) but then I figured this may not be the best idea because even though at this moment they are very similar, they may diverge in the future and this will be a situation with an unmaintainable base class (or set of tools).

So, long story short, I don't like this huge code duplication (in UI and View classes) but at the same time, I'm worried that extracting the common pieces will not be good in the long run.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 31, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling f8d8d07 on i/6112 into 1eb4b4c on master.

@oleq oleq requested a review from jodator January 31, 2020 11:55
@jodator
Copy link
Copy Markdown
Contributor

jodator commented Feb 3, 2020

At first, I wanted to create a base class (classes) for all of them (and/or a tool to help compose them) but then I figured this may not be the best idea because even though at this moment they are very similar, they may diverge in the future and this will be a situation with an unmaintainable base class (or set of tools).

I had a similar feeling about all the command classes. There are two base classes but it is not a big deal to refactor them later if needed. The code duplication at this stage is OK IMO.

Copy link
Copy Markdown
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 could nitpick to that one thing which I actually change before merging. So there's a green light from me.

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.

Implement the table properties UI

5 participants