-
Notifications
You must be signed in to change notification settings - Fork 40
Implementing 'is' methods to classes related to view and to model #1736
Conversation
@@ -58,10 +58,11 @@ export default class RootElement extends Element { | |||
* @inheritDoc | |||
*/ | |||
is( type, name ) { | |||
const cutType = type.replace( 'model:', '' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line after var def.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what's "cut type"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's type without module:
at the beginning to simplify logic in further lines.
Statement like:
( ( type == 'rootElement' || type == 'module:rootElement' ) && name == this.name ) || super.is( type, name );
seems to be a little to complex in my opinion.
@Reinmar, should I remove |
It does not make sense to have |
As a rule of thumb, I'd say that we need those methods where we need those methods. |
Can you check which classes are used with |
Basically, classes like |
I take only set from
|
This is |
Also, please exclude any code from |
|
I'd guess that |
@scofalik only those entries exists for those two: ckeditor5-engine/src/model/schema.js Line 1073 in 1fbee48
ckeditor5-engine/src/model/writer.js Line 195 in 694ee89
|
Agree with @scofalik . |
So, to conclude:
And that's all for now. |
I've adjusted scope of this PR to cover only mentioned classes. |
src/model/liverange.js
Outdated
@@ -40,6 +40,13 @@ export default class LiveRange extends Range { | |||
this.stopListening(); | |||
} | |||
|
|||
/** | |||
* @inheritDoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You send people to Range#is()
which documents that:
Checks whether given object is of
range
type.
Which means that you still don't know how to use this method to check for a live range
src/model/position.js
Outdated
/** | ||
* Checks whether given object is of `position` type. | ||
* | ||
* Read more at {@link module:engine/model/node~Node#is `Node#is()`}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending people to Node#is()
from Position
(which isn't a node) seems confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But see my other comment below about mentioning that this is a convention.
src/view/downcastwriter.js
Outdated
* @param {module:engine/view/attributeelement~AttributeElement} attribute Attribute element to use as wrapper. | ||
* @returns {module:engine/view/range~Range} range Range after wrapping, spanning over wrapping attribute element. | ||
*/ | ||
* Wraps elements within range with provided {@link module:engine/view/attributeelement~AttributeElement AttributeElement}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why touching this file in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the first stage when I add is()
method, my editor fix those spaces by replacing them with tabs. So it sees to be leftover. I'll revert it.
src/view/range.js
Outdated
@@ -394,6 +394,17 @@ export default class Range { | |||
} | |||
} | |||
|
|||
/** | |||
* Checks whether given object is of `range` type following the convention set by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wording is a lot better. I complained in the other place that you link to Node#is()
without much of an explanation. Saying that "following the convention set by" is a good way out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires documentation polishing.
Please build the API docs and try to read them as the developer would. Land in various places in these API docs and try to figure out whether it's clear from the method description how to use it.
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
I've rewritten the description. I hope it's better now. :) |
Yes, it's definitely better. But there's now a lot of repetition of things which can be centralised. It's hard to find the place where this method could be described in a bit deeper way, but Node#is sounds acceptable. In all other places, I'd keep the documentation as short as possible, but still very easy to parse thanks to examples. Because of them you don't have to necessarily go to any other place to understand how to use it. But you can if you want more details. I applied changes to a couple of model objects. Could you update all the other model objects and the view ones accordingly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented.
Suggested merge commit message (convention)
Other: Implementing 'is' method to classes related to view and model. Closes ckeditor/ckeditor5#4479 .