Image captioning #48

szymonkups · 2017-02-02T16:01:23Z

Adds captioning feature.
Currently there is no placeholder when caption's nested editable is empty.
There are other issues with nested editables, that will be reported in proper repositories and fixed (keyboard support etc.).
Fixes ckeditor/ckeditor5#5048.

…nd of figure element.

Reinmar · 2017-02-07T18:08:48Z

Some issues to be resolved in the first place:

There are too few features in the manual test. It's hard to test what happens in the caption. Tere are neither image features loaded, nor other features.
When the selection is inside the nested editable, the widget should not have the focus outline. There can be just one thing focus at a time and, in such situation, it's the nested editable.
See the screenshot – on this branch the image is rendered over the toolbar, when on master it's below:

Reinmar · 2017-02-07T18:10:26Z

And tests fail after I merged master into this branch.

Reinmar · 2017-02-07T18:14:13Z

src/converters.js

@@ -57,6 +59,13 @@ export function viewToModelImage() {
 			modelImage.setAttribute( 'alt', viewImg.getAttribute( 'alt' ) );
 		}

+		// Convert children of converted view element and append them to `modelImage`.


Please link to https://github.com/ckeditor/ckeditor5-engine/issues/736

Reinmar · 2017-02-07T18:16:39Z

src/imagecaptioning/imagecaptioning.js

+ *
+ * @extends module:core/plugin~Plugin
+ */
+export default class ImageCaptioning extends Plugin {


Simply: ImageCaption. We have ImageStyles not ImageStyling.

Besides, since this plugin is empty now, you can merge the engine part into it. Unless... unless we see that adding the placeholder will require some strictly UI-related logic.

OTOH, by splitting the feature beforehand we stick to some pattern, so I'm not sure what would be better.

Reinmar · 2017-02-08T09:49:49Z

src/imagecaptioning/utils.js

+ */
+export function captionEditableCreator( viewDocument ) {
+	return () => {
+		const editable = new ViewEditableElement( 'figcaption', { contenteditable: true } ) ;


Space before ;.

Reinmar · 2017-02-08T09:50:00Z

src/imagecaptioning/utils.js

+ * @param {module:engine/view/document~Document} viewDocument
+ * @return {Function}
+ */
+export function captionEditableCreator( viewDocument ) {


editableCaptionCreator()

Reinmar · 2017-02-08T09:50:47Z

src/imagecaptioning/utils.js

+const captionSymbol = Symbol( 'imageCaption' );
+
+/**
+ * Returns function that creates caption editable element for given {@link module:engine/view/document~Document}.


"Returns a function", "for the given"

Reinmar · 2017-02-08T09:55:24Z

src/imagecaptioning/utils.js

+ * @param {module:engine/view/element~Element} viewElement
+ * @return {Boolean}
+ */
+export function isCaptionEditable( viewElement ) {


isEditableCaption to pair with the other method. I'm also thinking about isCaption and captionElementCreator. That "editable" part is confusing. Or maybe captionEditableElementCreator? I don't know... :D So maybe let's choose the shortest versions, WDYT?

I'll go with captionElementCreator and isCaption - sounds better and it's shorter too.

Reinmar · 2017-02-08T10:04:23Z

src/imagecaptioning/utils.js

+ * @return {module:engine/model/element~Element|null}
+ */
+export function getCaptionFromImage( imageModelElement ) {
+	for ( let node of imageModelElement.getChildren() ) {


Could be like:

Array.from( imageModelElement.getChildren() ).find( node => node.is( 'caption' ) )

If we do https://github.com/ckeditor/ckeditor5-engine/issues/809.

Reinmar · 2017-02-08T10:07:51Z

src/widget/widget.js

 		if ( !isWidget( widgetElement ) ) {
-			widgetElement = widgetElement.findAncestor( element => isWidget( element ) );
+			widgetElement = widgetElement.findAncestor( element => {


widgetElement.findAncestor( isWidget );

Reinmar · 2017-02-08T10:08:44Z

src/widget/widgetengine.js

+				const widget = editableElement.findAncestor( element => isWidget( element ) );
+
+				if ( widget ) {
+					widget.addClass( WIDGET_SELECTED_CLASS_NAME );


Do we use the I_AM_CONST notation? I don't remember now what we've decided. cc @pjasiun, @scofalik

This is complicated :P.
Instead of using stuff like:

const STICKS_TO_PREVIOUS = 0; const STICKS_TO_NEXT = 1; ... if ( this.stickiness == STICKS_TO_PREVIOUS ) { ... }

we decided to just use 'sticksToPrevious' string. But this works only for closed list of constant values and was done to get rid of imports.

I don't know whether we decided anything about package-scope/configuration constants like this. If I wrote that code, I'd go with widgetSelectedClassName but I can't say this is wrong either.

I can see we use the ALL_CAPS notation in some cases. INLINE_FILLER_LENGTH, BR_FILLER, NBSP_FILLER, DEFAULT_PRIORITY. Also, all but one export const use this notation. So let's stick to that.

pjasiun · 2017-02-08T14:10:56Z

Can't you, instead of createImageAttributeConverter and modelToViewAttributeConverter use model to view conversion builder with fromAttribute( ... ).toAttribute( ... );?

szymonkups · 2017-02-08T14:36:43Z

Can't you, instead of createImageAttributeConverter and modelToViewAttributeConverter use model to view conversion builder with fromAttribute( ... ).toAttribute( ... );?

Conversion builder does not allow to specify any information about attribute's element. I don't want to convert all src and alt attributes, only those from image elements.

Tests: CHecking if caption is added to limits set in schema.

szymonkups · 2017-02-20T08:08:47Z

Added caption to limits set in schema, test it further after closing ckeditor/ckeditor5-engine#825.

Reinmar · 2017-02-21T08:09:46Z

Everything works brilliantly!

Reinmar · 2017-02-21T17:20:41Z

I'm closing this PR despite the fact that the related tickets in engine and enter packages are still open. Let's not risk some conflicts.

I've spotted two issues with the image caption implementation: #58 and #57.

szymonkups added 24 commits January 23, 2017 18:01

Added initial ImageCaptioningEngine and manual test.

2d143c4

Added conversion for elements nested inside image figure element.

6c83c6b

Image captioning engine and manual test.

00318cb

Image captioning converters.

6196dc2

Use EditableElement for nested editables.

ffd123e

Added tests to captioning converters.

5eace2c

Image captioning: model to view converter inserts figcaption at the e…

1bdf50e

…nd of figure element.

Added support for nested editables when clicking on widget.

b2eb5ed

Adding focus class to captioning nested editable.

1db21af

Merge branch 'master' into t/28

6392d6e

Adding view caption element when image widget is selected.

67c38ed

Removing empty image caption when image widget is no longer selected.

291125b

Update image captioning focus style.

586f8e6

Refactoring image captioning code.

afddbcf

More refactoring in image captioning.

4abc44f

Tests form ImageCaptioningEngine.

e987db7

Added more test to ImageCaptioningEngine.

7ec7dcf

Added tests for image captioning utils.

996df20

More tests to ImageCaptioningEngine.

064f957

Updated widget tests for integration with nested editables.

13bd1cc

Docs fixes in image captioning.

eda5d7a

Added tests for ImageCaptioning plugin.

d3c0f59

Updated image captioning manual test.

766f0cb

Merge branch 'master' into t/28

da46641

szymonkups requested a review from Reinmar February 2, 2017 16:01

Merge branch 'master' into t/28

760d434

Reinmar reviewed Feb 7, 2017

View reviewed changes

Reinmar reviewed Feb 8, 2017

View reviewed changes

Reinmar added the status:review- label Feb 8, 2017

szymonkups added 8 commits February 9, 2017 21:59

Merge branch 'master' into t/28

09bdb3f

Fixed image converters tests.

35cb5ed

Renamed ImageCaptioning to ImageCaption.

c290e29

Minor fixes in ImageCaption.

e4a3a66

Methods renaming, small fixes in ImageCaption.

045d7c4

Added more features to image caption manual test.

5aa3ffa

Removed focus class from image when caption editable is focused.

3ac2144

Better checking if clicked inside nested edtiable in widget.

b304d33

szymonkups removed the status:review- label Feb 10, 2017

Reinmar and others added 2 commits February 13, 2017 18:14

Tests: Added list feature to the manual test.

9d6e6c7

Other: Added caption to limits set in schema.

2603f00

Tests: CHecking if caption is added to limits set in schema.

Reinmar mentioned this pull request Feb 21, 2017

📅 News and updates: February 21, 2017 ckeditor/ckeditor5-design#174

Open

Reinmar added 2 commits February 21, 2017 17:02

Removed mistakenly committed it.only().

f67581a

Added a comment about TODO.

86dbdf2

Reinmar merged commit 6bb4069 into master Feb 21, 2017

Reinmar deleted the t/28 branch February 21, 2017 17:21

Reinmar mentioned this pull request Oct 9, 2019

Nested editables must have their classes ckeditor/ckeditor5#5067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image captioning #48

Image captioning #48

szymonkups commented Feb 2, 2017 •

edited by pomek

Loading

Reinmar commented Feb 7, 2017

Reinmar commented Feb 7, 2017

Reinmar Feb 7, 2017

Reinmar Feb 7, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

szymonkups Feb 10, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

Reinmar Feb 8, 2017

scofalik Feb 8, 2017 •

edited

Loading

Reinmar Feb 8, 2017

pjasiun commented Feb 8, 2017

szymonkups commented Feb 8, 2017 •

edited

Loading

szymonkups commented Feb 20, 2017

Reinmar commented Feb 21, 2017

Reinmar commented Feb 21, 2017

Image captioning #48

Image captioning #48

Conversation

szymonkups commented Feb 2, 2017 • edited by pomek Loading

Reinmar commented Feb 7, 2017

Reinmar commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun commented Feb 8, 2017

szymonkups commented Feb 8, 2017 • edited Loading

szymonkups commented Feb 20, 2017

Reinmar commented Feb 21, 2017

Reinmar commented Feb 21, 2017

szymonkups commented Feb 2, 2017 •

edited by pomek

Loading

scofalik Feb 8, 2017 •

edited

Loading

szymonkups commented Feb 8, 2017 •

edited

Loading