Introduced standalone view-to-model converter for img element. #97
Conversation
…t. View-to-model figure element converter do not create model image element on it's own.
|
Will need a PR in engine... |
src/image/imageengine.js
Outdated
| @@ -52,8 +54,18 @@ export default class ImageEngine extends Plugin { | |||
| createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'src' ); | |||
| createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'alt' ); | |||
|
|
|||
| // Build converter for view img element to model image element. | |||
| buildViewConverter().for( data.viewToModel ) | |||
| .from( { name: 'img', attribute: { src: /.+/ } } ) | |||
There was a problem hiding this comment.
src attribute is required to correctly convert view <img> to model <image> (which requires src attribute). If I understand the code correctly :), this was also implemented in "old" converter (before changes introduced in this PR): https://github.com/ckeditor/ckeditor5-image/pull/97/files#diff-ac5209f1bc4dd7be6f3619d8632013c9L43 here and below.
There was a problem hiding this comment.
So, being picky, I should just mention that . doesn't match new lines.
There was a problem hiding this comment.
/[\s\S]+/ is what we usually use. On the other hand, you didn't add ^ and $ so, in fact, if that regexp is later used with match(), then . alone would be enough :D
There was a problem hiding this comment.
being picky
Being PK-y?
There was a problem hiding this comment.
😆
Do you want to change this regexp to /./)? Or is that too PK-y?
There was a problem hiding this comment.
/./ is fine, but let's please close this already.
|
Testes passes after related PR in engine has been merged. |
|
Two small things:
|
Tests: added test checking converters behaviour for <figure> with multiple <img> elements.
|
Fixed |
src/image/imageengine.js
Outdated
| @@ -57,12 +57,13 @@ export default class ImageEngine extends Plugin { | |||
| // Build converter for view img element to model image element. | |||
| buildViewConverter().for( data.viewToModel ) | |||
| .from( { name: 'img', attribute: { src: /./ } } ) | |||
| .toElement( viewImage => new ModelElement( 'image', { src: viewImage.getAttribute( 'src' ) } ) ); | |||
| .toElement( ( viewImage ) => new ModelElement( 'image', { src: viewImage.getAttribute( 'src' ) } ) ); | |||
There was a problem hiding this comment.
You know I was removing these parenthesis in the previous commit? :D
There was a problem hiding this comment.
Current standard is having parenthesis around arguments of every arrow function.
tests/image/imageengine.js
Outdated
| @@ -147,6 +147,28 @@ describe( 'ImageEngine', () => { | |||
| expect( getModelData( document, { withoutSelection: true } ) ) | |||
| .to.equal( '<image alt="alt text" src="foo.png"></image>' ); | |||
| } ); | |||
|
|
|||
| it( 'should not convert alt attribute on non-img element', () => { | |||
There was a problem hiding this comment.
This test doesn't check anything. Check out src to the previous commit and it will also pass.
There was a problem hiding this comment.
Well, two things:
- Allowing alt on div is required, that's true. Without this the test doesn't make much sense.
- It will pass on previous version, because that was a feature of previous version too.
| const data = editor.data; | ||
| const editing = editor.editing; | ||
|
|
||
| document.schema.registerItem( 'div', '$block' ); |
There was a problem hiding this comment.
Maybe it's missing alt allowed here?
Suggested merge commit message (convention)
Feature: Introduced view-to-model conversion for bare
<img>elements. Closes ckeditor/ckeditor5#5032.