Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Update logic for get 'img' form 'figure'. #342

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

kaluginserg
Copy link
Contributor

@kaluginserg kaluginserg commented Feb 19, 2020

Update logic for get img form figure view element.
Fix strange behaviour with custom extending plugins with downcasting images attributes.
Issue describes here: ckeditor/ckeditor5#6294

Default behaviour for getting img-view element was : .getChild( 0 ) it is not best when we have some tags before img 'tag'. It can be possible in future...

Suggested behaviour:
Array.from( figureView.getChildren() ).find( viewChild => viewChild.is( 'img' ) );

and encapsulate it to utils function getImgViewFromFigure().

This approach provide ability to extend plugin logic with other custom plugins.


sergei.kalugin added 2 commits February 19, 2020 18:51
Fix errors with downcast img attributes,
and unsafe behabiour.
@kaluginserg kaluginserg requested review from jodator, dkonopka and Reinmar and removed request for jodator February 19, 2020 16:08
@kaluginserg
Copy link
Contributor Author

@Reinmar @dkonopka
continuous-integration build failed - but detailed reason seems not related with MergeRequest itself.
Can you review my Merge Request ?
Thanks.

@jodator jodator requested review from jodator and removed request for dkonopka and Reinmar February 20, 2020 11:01
@jodator jodator self-assigned this Feb 20, 2020
@jodator
Copy link
Contributor

jodator commented Feb 20, 2020

Hello - thanks for the fix we spotted that error but haven't had a use-case for such extension so it was left as-is.

I didn't check if it solves all of the issues with extending the image feature output, but the change itself looks OK.

If you could add a test for this would be awesome. I'll check this next week after the release process and most likely merge it.

@Reinmar
Copy link
Member

Reinmar commented Feb 20, 2020

As for the CI, please ignore that. It's something we'll need to resolve.

@kaluginserg
Copy link
Contributor Author

kaluginserg commented Feb 21, 2020

@jodator
About test cases I have no test cases, I think - needs to create some little extender, or sub-plugin, like current figcaption (but added before img tag)...

I can share with you our company concept for title/legends for images/tables, (it is almost work for images, with little bugs, but not work for tables itself, for tables we use this approach with extra-wrapper, because built-in table - hard extensible):
https://gist.github.com/kaluginserg/497d5702d1444d1284f660ec5cc27083

jodator added a commit that referenced this pull request Feb 26, 2020
Fix: The image converters should not assume that <img> is a first child of a <figure>. Closes ckeditor/ckeditor5#6294.
@jodator jodator merged commit 7dc430f into ckeditor:master Feb 26, 2020
@jodator
Copy link
Contributor

jodator commented Feb 26, 2020

Thank you @kaluginserg for fixing this. I've added one test to this. You can track the next release announcements here: ckeditor/ckeditor5#6317

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants