T/501 Allow opening dialog for an empty anchor #564
Conversation
plugins/link/plugin.js
Outdated
| @@ -94,10 +94,11 @@ | |||
| editor.on( 'doubleclick', function( evt ) { | |||
| // If the link has descendants and the last part of it is also a part of a word partially | |||
| // unlinked, clicked element may be a descendant of the link, not the link itself. (http://dev.ckeditor.com/ticket/11956) | |||
| var element = CKEDITOR.plugins.link.getSelectedLink( editor ) || evt.data.element.getAscendant( 'a', 1 ); | |||
| // evt.data.element condition allows opening anchor dialog if the anchor is empty (#501). | |||
| var element = CKEDITOR.plugins.link.getSelectedLink( editor ) || evt.data.element.getAscendant( 'a', 1 ) || evt.data.element; | |||
There was a problem hiding this comment.
I wonder if this condition could be simplified, e.g. setting element only if evt.data.element is a or img.
There was a problem hiding this comment.
@Comandeer PTAL 👀
plugins/link/plugin.js
Outdated
| if ( element && !element.isReadOnly() ) { | ||
| if ( element.is( 'a' ) ) { | ||
| if ( element ) { | ||
| if ( element.is( 'a' ) && !element.isReadOnly() ) { |
There was a problem hiding this comment.
Why this condition is moved here?
There was a problem hiding this comment.
This is a remnant of the experiments. I moved it to the right place :)
|
|
||
| **Unexpected result:** | ||
|
|
||
| Nothing happend. |
plugins/link/plugin.js
Outdated
| @@ -94,7 +94,8 @@ | |||
| editor.on( 'doubleclick', function( evt ) { | |||
| // If the link has descendants and the last part of it is also a part of a word partially | |||
| // unlinked, clicked element may be a descendant of the link, not the link itself. (http://dev.ckeditor.com/ticket/11956) | |||
There was a problem hiding this comment.
Please adjust the comment to standard format ...descendant of the link, not the link itself (http://dev.ckeditor.com/ticket/11956). (I know it was not changed, but we should correct it anyway).
plugins/link/plugin.js
Outdated
| @@ -94,7 +94,8 @@ | |||
| editor.on( 'doubleclick', function( evt ) { | |||
| // If the link has descendants and the last part of it is also a part of a word partially | |||
| // unlinked, clicked element may be a descendant of the link, not the link itself. (http://dev.ckeditor.com/ticket/11956) | |||
| var element = CKEDITOR.plugins.link.getSelectedLink( editor ) || evt.data.element.getAscendant( 'a', 1 ); | |||
| // evt.data.element condition allows opening anchor dialog if the anchor is empty (#501). | |||
There was a problem hiding this comment.
Try to always start a sentence with uppercase. In this case you could use The so The evt.data.element ....
tests/plugins/link/anchor.js
Outdated
|
|
||
| editor.on( 'dialogShow', function( evt ) { | ||
| resume( function() { | ||
| assert.areSame( evt.data._.name, 'anchor' ); |
There was a problem hiding this comment.
You can pass 3rd parameter as an assert description. In this case it could be useful because it might not be clear at the first glance what this assert is checking. It could look something like:
assert.areSame( evt.data._.name, 'anchor', 'Anchor dialog has been opened.' )
|
@Comandeer, please check if all your requests were resolved. |
plugins/link/plugin.js
Outdated
| var element = CKEDITOR.plugins.link.getSelectedLink( editor ) || evt.data.element.getAscendant( 'a', 1 ); | ||
| // unlinked, clicked element may be a descendant of the link, not the link itself (http://dev.ckeditor.com/ticket/11956). | ||
| // The evt.data.element.getAscendant( 'img', 1 ) condition allows opening anchor dialog if the anchor is empty (#501). | ||
| var element = evt.data.element.getAscendant( 'a', 1 ) || evt.data.element.getAscendant( 'img', 1 ); |
There was a problem hiding this comment.
Couldn't it be further simplified to:
evt.data.element.getAscendant( { a: 1, img: 1 }, true );?
There was a problem hiding this comment.
True, it looks better this way :) Changed 👍 PTAL @Comandeer
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
Yes
This PR contains
What changes did you make?
Add a new condition to element variable on double-click.
Closes #501