-
Notifications
You must be signed in to change notification settings - Fork 19
I/6049: Introduced the table cell properties UI #227
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.
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 :)
const editor = this.editor; | ||
const data = {}; | ||
|
||
for ( const propertyName of CELL_PROPERTIES ) { |
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.
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
}
import ToolbarView from '@ckeditor/ckeditor5-ui/src/toolbar/toolbarview'; | ||
import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview'; | ||
|
||
import checkIcon from '@ckeditor/ckeditor5-core/theme/icons/check.svg'; |
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.
❤️ 📜 I just love those wall of text in this whole feature :D
bottom: t( 'Align cell text to the bottom' ) | ||
}; | ||
} | ||
} |
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.
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 :)
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.
ps.: Also the above comment can have little sense if we can't have a need for enabling or disabling some properties.
src/ui/utils.js
Outdated
* @returns {module:utils/dom/position~Options} | ||
*/ | ||
export function getBalloonCellPositionData( editor ) { | ||
const model = editor.model; |
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.
I think that some of the intermediate values could be dropped in favor of a shorter method.
import TableCellPropertiesUI from '../../src/tablecellproperties/tablecellpropertiesui'; | ||
import TableCellPropertiesUIView from '../../src/tablecellproperties/ui/tablecellpropertiesview'; | ||
|
||
describe( 'TableCellPropertiesUI', () => { |
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.
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:
ps.: I'll hijack this comment for standup tension as current table tests are a mes :D
Self.R-: Use |
Nah, checked it and it's not necessary after all. |
Feature: Created the `LabeledView` class (see ckeditor/ckeditor5-table#227). Also added `id` properties to the `DropdownView` and `LabelView` for compatibility with the `LabeledView`.
Feature: Added styles for the LabeledView (see ckeditor/ckeditor5-table#227).
Feature: Added vertical alignment icons. Also moved alignment icons form ckeditor5-alignment (see ckeditor/ckeditor5-table#227).
Other: Moved alignment icons to ckeditor5-core (see ckeditor/ckeditor5-table#227). BREAKING CHANGE: The `align-left`, `align-right`, `align-center`, and `align-justify` icons have been moved to `ckeditor5-core`.
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.
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.
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
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)
Feature: Introduced the table cell properties UI. Closes ckeditor/ckeditor5#6049.
Additional information
Requirements
Dependencies:
LabeledView
and tweaked theDropdownView
andLabelView
LabeledView
Round up
New things in the PR:
TableCellPropertiesUI
(controller plugin)TableCellPropertiesView
(form view)FormRowView
a class shared between theTableCellPropertiesView
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).Followups:
Also:
LabeledInputView
in favor ofLabeledView