Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Removed LinkElement #163

Merged
merged 1 commit into from
Jan 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import ClickObserver from '@ckeditor/ckeditor5-engine/src/view/observer/clickobserver';
import Range from '@ckeditor/ckeditor5-engine/src/view/range';
import LinkEngine from './linkengine';
import LinkElement from './linkelement';
import { isLinkElement } from './utils';
import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon';

import clickOutsideHandler from '@ckeditor/ckeditor5-ui/src/bindings/clickoutsidehandler';
Expand Down Expand Up @@ -323,15 +323,15 @@ export default class Link extends Plugin {
}

/**
* Returns the {@link module:link/linkelement~LinkElement} under
* Returns the link {@link module:engine/view/attributeelement~AttributeElement} under
* the {@link module:engine/view/document~Document editing view's} selection or `null`
* if there is none.
*
* **Note**: For a non–collapsed selection the `LinkElement` is only returned when **fully**
* **Note**: For a non–collapsed selection the link element is only returned when **fully**
* selected and the **only** element within the selection boundaries.
*
* @private
* @returns {module:link/linkelement~LinkElement|null}
* @returns {module:engine/view/attributeelement~AttributeElement|null}
*/
_getSelectedLinkElement() {
const selection = this.editor.editing.view.selection;
Expand All @@ -340,7 +340,7 @@ export default class Link extends Plugin {
return findLinkElementAncestor( selection.getFirstPosition() );
} else {
// The range for fully selected link is usually anchored in adjacent text nodes.
// Trim it to get closer to the actual LinkElement.
// Trim it to get closer to the actual link element.
const range = selection.getFirstRange().getTrimmed();
const startLink = findLinkElementAncestor( range.start );
const endLink = findLinkElementAncestor( range.end );
Expand All @@ -349,7 +349,7 @@ export default class Link extends Plugin {
return null;
}

// Check if the LinkElement is fully selected.
// Check if the link element is fully selected.
if ( Range.createIn( startLink ).getTrimmed().isEqual( range ) ) {
return startLink;
} else {
Expand All @@ -359,11 +359,11 @@ export default class Link extends Plugin {
}
}

// Returns a `LinkElement` if there's one among the ancestors of the provided `Position`.
// Returns a link element if there's one among the ancestors of the provided `Position`.
//
// @private
// @param {module:engine/view/position~Position} View position to analyze.
// @returns {module:link/linkelement~LinkElement|null} LinkElement at the position or null.
// @returns {module:engine/view/attributeelement~AttributeElement|null} Link element at the position or null.
function findLinkElementAncestor( position ) {
return position.getAncestors().find( ancestor => ancestor instanceof LinkElement );
return position.getAncestors().find( ancestor => isLinkElement( ancestor ) );
}
20 changes: 0 additions & 20 deletions src/linkelement.js

This file was deleted.

11 changes: 2 additions & 9 deletions src/linkengine.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
import LinkElement from './linkelement';
import LinkCommand from './linkcommand';
import UnlinkCommand from './unlinkcommand';
import { createLinkElement } from './utils';

/**
* The link engine feature.
Expand All @@ -36,14 +36,7 @@ export default class LinkEngine extends Plugin {
// Build converter from model to view for data and editing pipelines.
buildModelConverter().for( data.modelToView, editing.modelToView )
.fromAttribute( 'linkHref' )
.toElement( linkHref => {
const linkElement = new LinkElement( 'a', { href: linkHref } );

// https://github.com/ckeditor/ckeditor5-link/issues/121
linkElement.priority = 5;

return linkElement;
} );
.toElement( linkHref => createLinkElement( linkHref ) );

// Build converter from view to model for data pipeline.
buildViewConverter().for( data.viewToModel )
Expand Down
38 changes: 38 additions & 0 deletions src/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module link/utils
*/

import AttributeElement from '@ckeditor/ckeditor5-engine/src/view/attributeelement';

const linkElementSymbol = Symbol( 'linkElement' );

/**
* Returns `true` if a given view node is the link element.
*
* @param {module:engine/view/node~Node} node
* @return {Boolean}
*/
export function isLinkElement( node ) {
return node.is( 'attributeElement' ) && !!node.getCustomProperty( linkElementSymbol );
}

/**
* Creates link {@link module:engine/view/attributeelement~AttributeElement} with provided `href` attribute.
*
* @param {String} href
* @return {module:engine/view/attributeelement~AttributeElement}
*/
export function createLinkElement( href ) {
const linkElement = new AttributeElement( 'a', { href } );
linkElement.setCustomProperty( linkElementSymbol, true );

// https://github.com/ckeditor/ckeditor5-link/issues/121
linkElement.priority = 5;

return linkElement;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate file for these methods? I mean they are used only once and I don't think this code will be used again. I think we could just keep it in-place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think that link will never be created by any other feature? I actually think that the chance is very high for this (by 3rd party code).

14 changes: 7 additions & 7 deletions tests/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -606,14 +606,14 @@ describe( 'Link', () => {
sinon.assert.calledWithExactly( spy );
} );

it( 'should open when selection exclusively encloses a LinkElement (#1)', () => {
it( 'should open when selection exclusively encloses a link element (#1)', () => {
setModelData( editor.model, '[<$text linkHref="url">foo</$text>]' );

observer.fire( 'click', { target: {} } );
sinon.assert.calledWithExactly( spy );
} );

it( 'should open when selection exclusively encloses a LinkElement (#2)', () => {
it( 'should open when selection exclusively encloses a link element (#2)', () => {
setModelData( editor.model, '<$text linkHref="url">[foo]</$text>' );

observer.fire( 'click', { target: {} } );
Expand All @@ -627,35 +627,35 @@ describe( 'Link', () => {
sinon.assert.notCalled( spy );
} );

it( 'should not open when selection is non-collapsed and doesn\'t enclose a LinkElement (#1)', () => {
it( 'should not open when selection is non-collapsed and doesn\'t enclose a link element (#1)', () => {
setModelData( editor.model, '<$text linkHref="url">f[o]o</$text>' );

observer.fire( 'click', { target: {} } );
sinon.assert.notCalled( spy );
} );

it( 'should not open when selection is non-collapsed and doesn\'t enclose a LinkElement (#2)', () => {
it( 'should not open when selection is non-collapsed and doesn\'t enclose a link element (#2)', () => {
setModelData( editor.model, '<$text linkHref="url">[fo]o</$text>' );

observer.fire( 'click', { target: {} } );
sinon.assert.notCalled( spy );
} );

it( 'should not open when selection is non-collapsed and doesn\'t enclose a LinkElement (#3)', () => {
it( 'should not open when selection is non-collapsed and doesn\'t enclose a link element (#3)', () => {
setModelData( editor.model, '<$text linkHref="url">f[oo]</$text>' );

observer.fire( 'click', { target: {} } );
sinon.assert.notCalled( spy );
} );

it( 'should not open when selection is non-collapsed and doesn\'t enclose a LinkElement (#4)', () => {
it( 'should not open when selection is non-collapsed and doesn\'t enclose a link element (#4)', () => {
setModelData( editor.model, 'ba[r<$text linkHref="url">foo]</$text>' );

observer.fire( 'click', { target: {} } );
sinon.assert.notCalled( spy );
} );

it( 'should not open when selection is non-collapsed and doesn\'t enclose a LinkElement (#5)', () => {
it( 'should not open when selection is non-collapsed and doesn\'t enclose a link element (#5)', () => {
setModelData( editor.model, 'ba[r<$text linkHref="url">foo</$text>]' );

observer.fire( 'click', { target: {} } );
Expand Down
13 changes: 0 additions & 13 deletions tests/linkelement.js

This file was deleted.

7 changes: 4 additions & 3 deletions tests/linkengine.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@

import LinkEngine from '../src/linkengine';
import LinkCommand from '../src/linkcommand';
import LinkElement from '../src/linkelement';
import UnlinkCommand from '../src/unlinkcommand';

import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
import { isLinkElement } from '../src/utils';

import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';

Expand Down Expand Up @@ -99,10 +99,11 @@ describe( 'LinkEngine', () => {
expect( getViewData( editor.editing.view, { withoutSelection: true } ) ).to.equal( '<p><a href="url">foo</a>bar</p>' );
} );

it( 'should convert to `LinkElement` instance', () => {
it( 'should convert to link element instance', () => {
setModelData( model, '<paragraph><$text linkHref="url">foo</$text>bar</paragraph>' );

expect( editor.editing.view.getRoot().getChild( 0 ).getChild( 0 ) ).to.be.instanceof( LinkElement );
const element = editor.editing.view.getRoot().getChild( 0 ).getChild( 0 );
expect( isLinkElement( element ) ).to.be.true;
} );

// https://github.com/ckeditor/ckeditor5-link/issues/121
Expand Down
43 changes: 43 additions & 0 deletions tests/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

import AttributeElement from '@ckeditor/ckeditor5-engine/src/view/attributeelement';
import ContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement';
import Text from '@ckeditor/ckeditor5-engine/src/view/text';

import { createLinkElement, isLinkElement } from '../src/utils';

describe( 'utils', () => {
describe( 'isLinkElement', () => {
it( 'should return true for elements created by createLinkElement', () => {
const element = createLinkElement( 'http://ckeditor.com' );

expect( isLinkElement( element ) ).to.be.true;
} );

it( 'should return false for other AttributeElements', () => {
expect( isLinkElement( new AttributeElement( 'a' ) ) ).to.be.false;
} );

it( 'should return false for ContainerElements', () => {
expect( isLinkElement( new ContainerElement( 'p' ) ) ).to.be.false;
} );

it( 'should return false for text nodes', () => {
expect( isLinkElement( new Text( 'foo' ) ) ).to.be.false;
} );
} );

describe( 'createLinkElement', () => {
it( 'should create link AttributeElement', () => {
const element = createLinkElement( 'http://cksource.com' );

expect( isLinkElement( element ) ).to.be.true;
expect( element.priority ).to.equal( 5 );
expect( element.getAttribute( 'href' ) ).to.equal( 'http://cksource.com' );
expect( element.name ).to.equal( 'a' );
} );
} );
} );