Ticket #5022 (closed Task: wontfix)

Opened 4 years ago

Last modified 4 years ago

Suggestion: Move functions "is()" and "getName()" to node.js

Reported by: Scott Owned by:
Priority: Normal Milestone:
Component: General Version:
Keywords: Discussion Cc:

Description

Hello,

During my tinkering with ckeditor, I think it would be very convenient to have the methods "is()" and "getName()" moved from element.js to node.js.

This makes sense because nodeName is a part of the browsers TextNode object as well. It helps because if you are doing a test to see if an element is say a <br /> the code changes from:

if(element && element.type == CKEDITOR.NODE_ELEMENT && element.is('br'))
or:
if(element && element.is && element.is('br'))

to just:
if(element && element.is('br'))

I have seen these sort of checks a lot of places in the ckeditor source. Let me know your thoughts.

Change History

comment:1 Changed 4 years ago by fredck

  • Keywords Discussion added

We wanted to have a clean and useful DOM representation in our API. We're not based on the W3C DOM for that, but we try to make them similar whenever possible. But, we're also avoiding senseless things.

While your idea makes sense to make those checks a bit simpler, looking at each of the API classes separately could make it senseless. For example, if you have a text node with the text value "br", one could thing that textnode.is('br') should return "true". After all its a text (string) being compared with a string.

Then, getName() for text or comment nodes is again senseless, and it's a duplication of something that can be achieved through the "type" property.

Of topic... Regarding your code, I know you may find "element.is" to check for element nodes, but this is wrong. The "type" attribute should always be used for node type detection. I've already had a talk with the other guys about that.

comment:2 Changed 4 years ago by fredck

  • Type changed from Bug to Task

comment:3 Changed 4 years ago by Scott

Understand your point with the text node .is() being misunderstood to be the text content.. But... CKEDITOR.dom.text.is is undefined.

Maybe the method .is() should be renamed to .isA(), isName(), isNode() or isNodeName()?

Anyway - its not a big deal, just a suggestion as I was working with it :)

comment:4 Changed 4 years ago by fredck

  • Status changed from new to closed
  • Resolution set to wontfix

There are no strong benefits on making changes to the API at this point. We'll keep this in mind if we find out it's a critical need.

Thanks for the suggestion anyway ;)

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy