Added CKEDITOR.config.image2_maxSize #2683
Conversation
plugins/image2/plugin.js
Outdated
| updateData = true; | ||
| } else { | ||
| updateData = false; | ||
| if ( isAllowedSize() ) { |
There was a problem hiding this comment.
IMO it would be more readable if isAllowedSize required new width and height as parameters.
plugins/image2/plugin.js
Outdated
| maxSize = CKEDITOR.tools.copy( maxSize ); | ||
| natural = CKEDITOR.plugins.image2.getNatural( image ); | ||
|
|
||
| maxSize.width = Math.max( maxSize.width === 'naturalWidth' ? natural.width : maxSize.width, min.width ); |
There was a problem hiding this comment.
Wouldn't be better to accept natural as a keyword? It'd be the same for both width and height.
plugins/image2/plugin.js
Outdated
|
|
||
| function isAllowedSize() { | ||
| var isTooSmall = newWidth < min.width || newHeight < min.height, | ||
| isTooBig = max && ( ( max.width && newWidth > max.width ) || ( max.height && newHeight > max.height ) ); |
There was a problem hiding this comment.
Doesn't the existence of max mean that both width and height has to be declared?
| @@ -0,0 +1,31 @@ | |||
| @bender-tags: 4.12.0, feature, bug | |||
There was a problem hiding this comment.
OTOH it seems a little bit strange to test both feature and bug in one test.
| bot.setData( '<img src="%BASE_PATH%/_assets/logo.png">', function() { | ||
| var image = editor.editable().findOne( 'img' ); | ||
|
|
||
| if ( CKEDITOR.env.chrome ) { |
There was a problem hiding this comment.
Is this branch necessary? For the sake of the simplicity, Chrome can also wait for image.
There was a problem hiding this comment.
Actually at this point Chrome unlike other browsers has image loaded, and waitForImage triggers on load event which won't happen in Chrome (#2714).
engineering-this
force-pushed
the
t/2048
branch
from
ad53abf
to
57227c8
Compare
| @@ -0,0 +1,15 @@ | |||
| @bender-tags: 4.12.0, feature, bug, 2672 | |||
There was a problem hiding this comment.
After splitting test into two separate ones, declaring them as feature & bug is no longer needed – one is for feature and one is for bug.
|
|
||
| ## Expected: | ||
|
|
||
| Dialog fields are populated with correct width/height. |
There was a problem hiding this comment.
It's not clear, which values are correct and which are not.
| }; | ||
|
|
||
| var tests = { | ||
| //2672 |
There was a problem hiding this comment.
Our convention for such comments is // (#<issue number>).
| }, | ||
|
|
||
| // #2714 | ||
| 'test waitForImage when image is loaded': function() { |
There was a problem hiding this comment.
This test should not be here, as it's connected to the test helper, not to the plugin itself. In fact helpers usually don't have tests.
|
…ouse button once image is at minimum size.
What is the purpose of this pull request?
New feature + Bugfix
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?
CKEDITOR.config.image2_maxSizethat limits maximum size that image can be resized with resizer.mousemovelistener to update widget data with last correct value which is same as one set on element.Closes #2048
Closes #2672
Closes #2714