Fixed resizer initial percentage values for sie images #308
Conversation
There was a problem hiding this comment.
👍 The PR itself is OK. I've added some comments about the tests here: https://github.com/ckeditor/ckeditor5-image/issues/304#issuecomment-523374354.
Only thing is that this https://github.com/ckeditor/ckeditor5-image/issues/314 is still an issue. (th3 #306 case looks solved).
|
Same as in ckeditor/ckeditor5-widget#103 – I'd like to look into this still so, removing R+ for now. |
tests/imageresize.js
Outdated
| describe( 'percent resizing', () => { | ||
| describe( 'standard image', () => { | ||
| beforeEach( async () => { | ||
| await editor.destroy(); |
There was a problem hiding this comment.
This is odd. Please create the right editor for the specific describe block. Creating 2 editors for no reason is both confusing and slows down the tests.
tests/imageresize.js
Outdated
| describe( 'percent resizing', () => { | ||
| describe( 'standard image', () => { | ||
| before( () => { | ||
| customConfig = CONFIG_RESIZE_IN_PIXELS; |
There was a problem hiding this comment.
You mean in percentage? Pixels are the default in this test file.
There was a problem hiding this comment.
BTW, we tend to simply repeat .create() calls in each test file instead of deduplicating it like this. It's easier to read the code if it's closer to you and doesn't have conditions inside.
There was a problem hiding this comment.
I'm not entirely sure about this one, it introduces notable code repetition across the file. Added changes in t/306b branch, can be compared to t/306.
There was a problem hiding this comment.
Tests are all about readability. If you need to jump to 10 places to understand what a single TC does, you have a problem. Therefore, repetition is better. Or rather – keeping as much code locally in a TC is better.
tests/imageresize.js
Outdated
| beforeEach( () => { | ||
| setData( editor.model, `<paragraph>foo</paragraph>[<image src="${ IMAGE_SRC_FIXTURE }"></image>]` ); | ||
|
|
||
| view = editor.editing.view; |
There was a problem hiding this comment.
This duplicates the code from the topmost beforeEach().
|
Pushed changes to |
Suggested merge commit message (convention)
Fix: Initial resize of a side image with no width predefined now gives correct percentage values. Closes ckeditor/ckeditor5#5190.
Additional information
Requires a following PR: ckeditor/ckeditor5-widget#103.