Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Node/DF#is method #3979

Closed
Reinmar opened this issue Feb 8, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-engine#827
Closed

Introduce Node/DF#is method #3979

Reinmar opened this issue Feb 8, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-engine#827
Assignees
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 8, 2017

We had such method in CKE4, but it was only available for elements: el.is( 'p' ). It's just a bit nicer than el.name == 'p', but a lot nicer than el.name == 'p' || el.name == 'div'.

What I've been thinking of is to make this method even more useful, by allowing checking whether the node is a text node or document fragment or whatever else.

There are many algorithms which need to verify what kind of node they are dealing with. We've seen e.g. the whole discussion around https://github.com/ckeditor/ckeditor5-engine/issues/807.

If we had a nicer API to check the type of the node or DF, then we'd be more pleased to use it.

Some ideas:

// If called with one arg, checks if this is an element with a given name:
node.is( 'p' );
// Equals to:
node.is( 'element', 'p' );

// Other types:
node.is( 'documentFragment' );
node.is( 'text' );
node.is( 'textProxy' );
node.is( 'text', 'expected data?' ); // not sure about this

// Matching more elements names:
node.is( [ 'p', 'div' ] ); // I don't like passing an array, so maybe simply...
node.is( 'p,div' );

Such method will have a lot of pros:

  1. Less code.
  2. More readable code.
  3. Less imports.
  4. Less potential circular deps.
@Reinmar
Copy link
Member Author

Reinmar commented Feb 8, 2017

Example how code would be simplified: https://github.com/ckeditor/ckeditor5-image/pull/48/files#r100029767

@scofalik
Copy link
Contributor

scofalik commented Feb 8, 2017

Either you implement is using ducktyping or you have to import everything in Node to check instanceof -- this won't solve circ deps, it will make it even worse.

The idea is neat and might clean code, but I am afraid it will require some dirty ducktyping or properties like type...

@Reinmar
Copy link
Member Author

Reinmar commented Feb 8, 2017

Either you implement is using ducktyping or you have to import everything in Node to check instanceof -- this won't solve circ deps, it will make it even worse.

Good point.

or properties like type...

Yep, exactly! I've been proposing this property long time ago anyway :)

Checking the types by the property and not instance of will also prevent some nasty issues cause by duplicated engine modules (may happen in some broken cases).

@scofalik
Copy link
Contributor

scofalik commented Feb 9, 2017

I mean.. Okay, I can live with this. This is language fault, not our fault, that importing is difficult and to check types we have to do such silly things.

Still, I'd be more happy if type property did not exist :)

@pjasiun
Copy link

pjasiun commented Feb 9, 2017

Or the is method might not be implemented only in node but in each class separately. Note that is( 'p' ) on element check tag name, but on text will return false.

@scofalik
Copy link
Contributor

scofalik commented Feb 9, 2017

This is actually good idea.

@pjasiun
Copy link

pjasiun commented Feb 9, 2017

BTW: it reminds me that we have a view.Matcher class. Having the is method we can make it work not only on the element but also on element types (can match all elements of given type).

@pjasiun
Copy link

pjasiun commented Feb 9, 2017

But to be honest I have mixed feelings about the Matcher class. Usually, it's more convenient for me to use a simple callback.

@scofalik scofalik self-assigned this Feb 15, 2017
@scofalik
Copy link
Contributor

I'll implement this feature using @pjasiun idea.

Of course (?) is method will be "inherited" like classes (so element.is( 'node' ) will return true).

@scofalik
Copy link
Contributor

Do we bring those for view too?

@Reinmar
Copy link
Member Author

Reinmar commented Feb 16, 2017

Do we bring those for view too?

I guess we'd like to keep this consistent.

Of course (?) is method will be "inherited" like classes (so element.is( 'node' ) will return true).

I'm not sure about that. This doesn't seem to be necessary. If there's is() method, then you can assume it's a node already. Let's KISS.

@scofalik
Copy link
Contributor

You are right about Node. What I mean is -- every class that extends Element should return true for .is( 'element' ).

Reinmar referenced this issue in ckeditor/ckeditor5-engine Feb 21, 2017
Feature: Introduced `is()` method in model and view tree nodes and document fragments. Closes #809.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 8 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants