Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods #1711

mlewand · 2019-03-28T14:16:58Z

Suggested merge commit message (convention)

Feature: Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods. Closes ckeditor/ckeditor5#1659.

Followups

- implement it in our formatting plugins (Moving node to the same position is handled by differ as a change ckeditor5#4478)
- - bold, italic, underline, strikethrough, code, subscript, superscript
- - fontSize, fontFamily
- - alignment
- - ~~highlight~~ I'll actually start a discussion whether highlight should be considered formatting markup or not.
- integrate it with remove formatting plugin
- integrate it in CKE5 inspector

…ributeProperties() methods.

…edef describing known properties.

…teProperties to a more proper place.

Reinmar

🔂 😛

Reinmar · 2019-03-28T15:36:11Z

src/model/schema.js

+		 * A map containing attribute's properties.
+		 *
+		 * @private
+		 * @member {Map.<String,String>}


This is a plain object, not a map. So Object.<String,String>

Reinmar · 2019-03-28T15:37:01Z

src/model/schema.js

-	 *
-	 * @returns {Object.<String,module:engine/model/schema~SchemaCompiledItemDefinition>}
-	 */
-	getDefinitions() {


Why this change? This method was here on purpose (next to getDefinition()).

As funny as it might sound I have grabbed one method too much where moving setAttributeProperties and getAttributeProperties. Thanks for catching that.

Restored to the original position.

Reinmar · 2019-03-28T15:38:05Z

src/model/schema.js

@@ -475,6 +470,70 @@ export default class Schema {
 		}, { priority: 'high' } );
 	}

+	/**
+	 * Registers custom properties to a given attribute.


Sets? Registering means saying "I exist!". We register nodes (Schema#register()). You can call register( foo ) only once too (cause you report your existence once).

Or rather:

Defines properties of the given attribute.

This method allows assigning additional information to model attributes. Currently, it is only used to mark formatting attributes (like bold, italic) which allows differentiating them from semantic attributes (such as src, listType, etc.). This knowledge is then used by features like "Remove format".

And that's it. Good documentation should back all big words by giving real-life examples.

I see you used "metadata" below. That's a nice word too. You may try to use it here too. It may be better than "information" or "properties".

The introduction was modified to shortly mention cases and link AttributeProperties#isFormatting example for further reading.

Unfortunately I could not link to isFormatting property directly due to ckeditor/ckeditor5-dev#504.

PTAL

Reinmar · 2019-03-28T15:38:28Z

src/model/schema.js

+	 *		} );
+	 *
+	 *		console.log( schema.getAttributeProperties( 'blockQuote' ) );
+	 *		// Logs: {one: 1, two: 2}


{ one: 1, two: 2 }

That was actually the output copied straight from the browser. I'll reformat this.

Reinmar · 2019-03-28T15:38:55Z

src/model/schema.js

+	 *			isFormatting: false
+	 *		} );
+	 *
+	 * You can also use custom attributes:


custom properties

Reinmar · 2019-03-28T15:39:01Z

src/model/schema.js

+	 * You can also use custom attributes:
+	 *
+	 *		schema.setAttributeProperties( 'blockQuote', {
+	 *			customAttribute: 'value'


customProperty

Reinmar · 2019-03-28T15:39:57Z

src/model/schema.js

+	 *
+	 * It can be used to mark the attributes relation and handle them in a common way.
+	 *
+	 *		// Mark blockQuote as a formatting attribute.


blockQuote is not an attribute – it's an element

Let's use something realistic – bold, italic, fontFamily, etc.

Reinmar · 2019-03-28T15:47:46Z

src/model/schema.js

+	}
+
+	/**
+	 * Returns properties assigned to a given attribute.


Returns properties of the given attribute.

Reinmar · 2019-03-28T15:48:19Z

src/model/schema.js

+ * See {@link module:engine/model/schema~Schema#setAttributeProperties `Schema#setAttributeProperties()`} for usage examples.
+ *
+ * @typedef {Object} module:engine/model/schema~AttributeProperties
+ * @property {Boolean} [isFormatting] Indicates that the attribute should be considered as a visual formatting.


Missing examples of what it means. Show what common attrs are considered "formatting" and which not.

oleq

Mostly changes in docs.

I also find the custom AttributeProperties data type confusing and it should be somehow resolved.

oleq · 2019-03-28T16:25:03Z

src/model/schema.js

@@ -39,6 +39,14 @@ export default class Schema {
 	constructor() {
 		this._sourceDefinitions = {};

+		/**
+		 * A map containing attribute's properties.


Suggested change

* A map containing attribute's properties.

* A map containing attribute properties.

oleq · 2019-03-28T16:25:43Z

src/model/schema.js

+		 * A map containing attribute's properties.
+		 *
+		 * @private
+		 * @member {Map.<String,String>}


Suggested change

* @member {Map.<String,String>}

* @member {Object.<String,String>}

oleq · 2019-03-28T16:26:19Z

src/model/schema.js

@@ -475,6 +470,70 @@ export default class Schema {
 		}, { priority: 'high' } );
 	}

+	/**
+	 * Registers custom properties to a given attribute.


Suggested change

* Registers custom properties to a given attribute.

* Adds custom properties to an attribute.

This was addressed by fix for one issues raised in previous review.

oleq · 2019-03-28T16:33:29Z

src/model/schema.js

+	/**
+	 * Registers custom properties to a given attribute.
+	 *
+	 * It can be used to mark the attributes relation and handle them in a common way.


Suggested change

* It can be used to mark the attributes relation and handle them in a common way.

* Properties allow categorization of model attributes. Thanks to that, certain groups of attributes can be recognized and handled in the same way by some of the editor features.

This was addressed by fix for one issues raised in previous review.

oleq · 2019-03-28T16:35:05Z

src/model/schema.js

+	 *			isFormatting: false
+	 *		} );
+	 *
+	 * You can also use custom attributes:


This is very confusing. We're talking about properties and all of the sudden there's this line about custom attributes of... attributes? This needs rewording. I guess the intention was that one can use any property name they want.

Yes it was a typo, should be custom properties.

oleq · 2019-03-28T16:38:09Z

src/model/schema.js

+	 *		console.log( schema.getAttributeProperties( 'blockQuote' ) );
+	 *		// Logs: {one: 1, two: 2}
+	 *
+	 * @param {String} attributeName Name of the attribute to receive properties.


Suggested change

* @param {String} attributeName Name of the attribute to receive properties.

* @param {String} attributeName A name of the attribute to gain the properties.

I'm not sure about receive -> gain change. I semantically both words are very similar, while receive is used more commonly.

I'll keep receive - but ofc feel free to follow this up.

oleq · 2019-03-28T16:40:04Z

src/model/schema.js

+	 *		// Logs: {one: 1, two: 2}
+	 *
+	 * @param {String} attributeName Name of the attribute to receive properties.
+	 * @param {module:engine/model/schema~AttributeProperties} properties A dictionary of properties.


This is confusing. First, it is stated that

You can also use custom attributes:

Then we enforce a type, which clearly declares which properties are allowed.

oleq · 2019-03-28T16:41:27Z

src/model/schema.js

+	 * @param {module:engine/model/schema~AttributeProperties} properties A dictionary of properties.
+	 */
+	setAttributeProperties( attributeName, properties ) {
+		this._attributeProperties[ attributeName ] = Object.assign( this._attributeProperties[ attributeName ] || {}, properties );


Suggested change

this._attributeProperties[ attributeName ] = Object.assign( this._attributeProperties[ attributeName ] || {}, properties );

this._attributeProperties[ attributeName ] = Object.assign( this.getAttributeProperties( attributeName ) || {}, properties );

oleq · 2019-03-28T16:42:04Z

src/model/schema.js

+	}
+
+	/**
+	 * Returns properties assigned to a given attribute.


Suggested change

* Returns properties assigned to a given attribute.

* Returns properties associated with a given model attribute. See {@link #setAttributeProperties `setAttributeProperties()`}.

oleq · 2019-03-28T16:44:19Z

src/model/schema.js

@@ -1285,6 +1344,15 @@ export class SchemaContext {
 * @typedef {Object} module:engine/model/schema~SchemaContextItem
 */

+/**
+ * A structure containing additional metadata describing the attribute.


As I wrote earlier, setAttributeProperties() allows any property names while this type clearly narrows possible options to isFormatting only. We should figure that out.

coveralls · 2019-03-28T22:32:22Z

Coverage remained the same at 100.0% when pulling 4688a17 on t/ckeditor5/1659 into bd875c7 on master.

I accidentally moved it too while reordering Schema#setAttributeProperties() method.

…ributes than specified in the AttributeProperties typedef.

mlewand · 2019-03-29T08:15:12Z

All the requested changes were addressed. There's one thing that requires some comments:

This is confusing. First, it is stated that

You can also use custom attributes:

Then we enforce a type, which clearly declares which properties are allowed.

This is interesting point @oleq and I can see how it can be an issue.

I can see two ways to resolve it:

Stick to a dedicated typedef and be explicit about the fact that it could be extended/patched (as basically everything in js world).
- Pros:
  - Reduced DRY factor.
  - Allows for a more detailed property documentation.
- Cons:
Describe the structure using inline object notation. (pushed an example to the t/ckeditor5/1659b branch)
- Pros:
  - The method description is closer to (g/s)etAttributeProperties.
- Cons:
  - I'm not even sure if our docs properly handle @returns tag with inline object. As it renders pretty 💩y.
  - Method API docs length increases, containing details that are not necessary to understand this method.

Taking above into consideration I'm strongly for going with a dedicated typedef, but explicitly say that it can be extended.

Reinmar · 2019-03-29T08:50:08Z

Perfect! ❤️

mlewand added 5 commits March 28, 2019 12:10

Feature: Introduced schema#setAttributeProperties() and schema#getAtt…

7cd8a43

…ributeProperties() methods.

Docs: Improved docs for setAttributeProperties and added a custom typ…

5b0ce2d

…edef describing known properties.

Code style: Moved schema#getAttributeProperties and schema#setAttribu…

72604d5

…teProperties to a more proper place.

Docs: Fixed API reference link.

0495b38

Tests: Simplified common code.

118b34c

mlewand requested a review from oleq March 28, 2019 14:47

Reinmar suggested changes Mar 28, 2019

View reviewed changes

oleq suggested changes Mar 28, 2019

View reviewed changes

mlewand added 3 commits March 29, 2019 08:44

Internal: Applied review corrections. [skip ci]

9983483

Code style: Restored getDefinitions to its rightful place. [skip ci]

99764b9

I accidentally moved it too while reordering Schema#setAttributeProperties() method.

Docs: More explicit information that it's acceptable to use other att…

4688a17

…ributes than specified in the AttributeProperties typedef.

mlewand requested a review from Reinmar March 29, 2019 08:15

Reinmar merged commit 1c6f83a into master Mar 29, 2019

Reinmar deleted the t/ckeditor5/1659 branch March 29, 2019 08:50

deonclem mentioned this pull request Oct 9, 2019

getAttributeProperties is not a function ckeditor/ckeditor5#2333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods #1711

Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods #1711

mlewand commented Mar 28, 2019 •

edited by pomek

Loading

Reinmar left a comment

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

mlewand Mar 29, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

mlewand Mar 29, 2019

Reinmar Mar 28, 2019

mlewand Mar 28, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

Reinmar Mar 28, 2019

oleq left a comment

oleq Mar 28, 2019

oleq Mar 28, 2019

oleq Mar 28, 2019

mlewand Mar 29, 2019

oleq Mar 28, 2019

mlewand Mar 29, 2019

oleq Mar 28, 2019

mlewand Mar 29, 2019

oleq Mar 28, 2019

mlewand Mar 29, 2019

oleq Mar 28, 2019

oleq Mar 28, 2019

oleq Mar 28, 2019

oleq Mar 28, 2019

coveralls commented Mar 28, 2019 •

edited

Loading

mlewand commented Mar 29, 2019

Reinmar commented Mar 29, 2019

	* A map containing attribute's properties.
	* A map containing attribute properties.

	* @member {Map.<String,String>}
	* @member {Object.<String,String>}

	* Registers custom properties to a given attribute.
	* Adds custom properties to an attribute.

	* It can be used to mark the attributes relation and handle them in a common way.
	* Properties allow categorization of model attributes. Thanks to that, certain groups of attributes can be recognized and handled in the same way by some of the editor features.

	* @param {String} attributeName Name of the attribute to receive properties.
	* @param {String} attributeName A name of the attribute to gain the properties.

	this._attributeProperties[ attributeName ] = Object.assign( this._attributeProperties[ attributeName ] \|\| {}, properties );
	this._attributeProperties[ attributeName ] = Object.assign( this.getAttributeProperties( attributeName ) \|\| {}, properties );

	* Returns properties assigned to a given attribute.
	* Returns properties associated with a given model attribute. See {@link #setAttributeProperties `setAttributeProperties()`}.

Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods #1711

Introduced schema#setAttributeProperties() and schema#getAttributeProperties() methods #1711

Conversation

mlewand commented Mar 28, 2019 • edited by pomek Loading

Suggested merge commit message (convention)

Followups

Reinmar left a 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

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

oleq left a 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

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

coveralls commented Mar 28, 2019 • edited Loading

mlewand commented Mar 29, 2019

Reinmar commented Mar 29, 2019

mlewand commented Mar 28, 2019 •

edited by pomek

Loading

coveralls commented Mar 28, 2019 •

edited

Loading