Add unit type selection for table cell height #3075
Conversation
There was a problem hiding this comment.
I see two issues, not a big deal, everything else is just refactoring and other code related issues.
-
Manual test is missing.
-
Unit pickers have different width, although they are the same and they are preceded with inputs of same width, can this be corrected?
About refactoring:
height and width dialog field definitions have methods which are almost identical: onLoad, setup, commit. It can be refactored by moving it's logic to helpers, that will return given callback. E.g.
setup: setupSizeField( 'width' )
However after looking at surrounding code I realised that whole definitions are almost identical, they differ only by property name which can be height or width. So we could go even further and do nice refactoring:
Replace
{
requiredContent: 'td{width}',
type: 'hbox',
widths: [ '70%', '30%' ],
children: [ {
...
},{
requiredContent: 'td{height}',
type: 'hbox',
widths: [ '70%', '30%' ],
children: [ {
...
},
With:
getCellSizeFieldDefinition( 'width' ),
getCellSizeFieldDefinition( 'height' ),
|
Problem with pickers width reported - #3098 |
Dumluregn
force-pushed
the
t/2084
branch
2 times, most recently
from
5aec7f7 to
5b70c6f
Compare
There was a problem hiding this comment.
New changes broke the existing layout:
- Open http://tests.ckeditor.test:1030/tests/plugins/tabletools/manual/allowedcontent
- Check "Width" and "Height".
- Open cell properties dialog.
Expected:
Everything's alright
Unexpected:
Wild scrollbar appears!
Please also report issue mentioned here: #3075 (comment)
|
Wild scrollbar tamed, it will no longer cause us trouble. I also fixed the changelog and reported side issues. |
…lugin' from links and delete unnecessary '4.11.5' version tag.
What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What changes did you make?
I copied the solution used for cell width.
Closes #2084.
Closes #3098.
Closes #3164.