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

Adding view image caption if not present when insertion to model caption is made #66

Merged
merged 10 commits into from
Mar 8, 2017

Conversation

szymonkups
Copy link
Contributor

@szymonkups szymonkups commented Mar 2, 2017

Suggested merge commit message (convention)

Fix: Adding view image caption if not present when insertion to model caption is made. Closes ckeditor/ckeditor5#5068.

@szymonkups szymonkups requested a review from Reinmar March 2, 2017 17:55
@szymonkups szymonkups changed the title T/58 Adding view image caption if not present when insertion to model caption is made Mar 2, 2017
@Reinmar
Copy link
Member

Reinmar commented Mar 3, 2017

R- for 3 reasons:

  1. There's no integration test with undo, so it's hard to understand whether the original issue was indeed fixed.
  2. There are too many functions exposed in imagecaption/utils. Some of them have really low value, like isInsideCaption() or insertViewCaptionAndBind(). Please remember that these utils make a public API, so whatever lands there should have some sense. Also, isInsideCaption() and insertViewCaptionAndBind() are now used in just one file and are very specific. The other utils may not be super needed now, but they are very generic and encapsulate some logic: isCaption(), getCaptionFromImage().
  3. You didn't explain how the code works and I can see that it works differently than what we discussed in #58.

@szymonkups
Copy link
Contributor Author

  1. You're right, I will add it.
  2. I've moved those methods to utilities because in my opinion the main file was unnecessarily growing. I also wanted to have separate tests for those specific methods. On the other hand, I can agree that when we treat utils as a public API some of the functions don't have much sense in other contexts.
  3. The essential change is adding insertMissingViewCaptionElement method to insert event listener. It does what you proposed in that thread:

Perhaps a solution would be to listen on conversion's insert event and if insert happens inside model's <caption>, then adding the <figcaption> in the view too. Am I right?

@Reinmar
Copy link
Member

Reinmar commented Mar 6, 2017

The essential change is adding insertMissingViewCaptionElement method to insert event listener. It does what you proposed in that thread:

Hm... I missed it in the code. Sorry.

I've moved those methods to utilities because in my opinion the main file was unnecessarily growing. I also wanted to have separate tests for those specific methods. On the other hand, I can agree that when we treat utils as a public API some of the functions don't have much sense in other contexts.

There's nothing wrong with long files as long as the code inside is written nicely. In fact, it's often easier to read the code if it's in one place than when it's split between multiple locations. Developers tend to over-optimise this because it's a typical evidence of premature optimisation. This leads to futher complication because exposed code creates public API and should be written in a more future-proof way.

imagecaptionengine has ~250LOC which is still a very short module, so feel free to drop there more code.

@Reinmar Reinmar merged commit 8e36645 into master Mar 8, 2017
@Reinmar Reinmar deleted the t/58 branch March 8, 2017 22:05
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.

Error when caption needs to be brought back on undo/redo
2 participants