-
Notifications
You must be signed in to change notification settings - Fork 18
Remove deprecated configuration option. #188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
table/table~TableConfig#toolbar
property API docs should also be handled properly. We have two options:
- we removed it all along,
- we clearly mark that it was removed in version x.y.z.
I'd go with removing it all along, to not keep zombie API entries. Another reason for this is that with our iteration-based approach we don't know what is the version number that we're about to release (technically looking at the warning message we should release v13.0.0).
- https://github.com/ckeditor/ckeditor5-table/blob/972a429/tests/tabletoolbar.js#L80 - there's no
config.table.toolbar
anymore - Proposed commit message should include
BREAKING CHANGE:
section.
tests/tabletoolbar.js
Outdated
@@ -239,12 +239,11 @@ describe( 'TableToolbar', () => { | |||
} ); | |||
|
|||
describe( 'deprecated toolbar', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't think we need unit tests for removing a variable. Instead we should remove the suite all along.
During this task I also found missing docs. I made a separate PR for that #189. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Suggested merge commit message (convention)
Other: Remove deprecated 'table.toolbar' configuration option. Closes ckeditor/ckeditor5#3259.
BREAKING CHANGE:
config.table.toolbar
is now removed from code. Useconfig.table.contentToolbar
instead.Additional information
I remain configuration docs entry for this option. I think there should be some trace that such option existed. However Umberto doesn't mark it properly.It was removed as well after review comment.Related issue ( https://github.com/cksource/umberto/issues/799 )