Mark nested widgets properly #58
Conversation
I moved a part of the logic to the separate method. Unfortunately, I have no idea how I could more improve the code. Any tip will be appreciated. |
| @@ -1210,5 +1210,74 @@ describe( 'Widget', () => { | |||
|
|
|||
| expect( getModelData( model ) ).to.equal( '<paragraph>bar</paragraph>[<widget></widget>]<paragraph>foo</paragraph>' ); | |||
| } ); | |||
|
|
|||
| it( 'should select the most top-outer widget if widgets are nested', () => { | |||
There was a problem hiding this comment.
Missing scenarios:
- multiple widgets (on the same level) in the selection,
- some additional element between the outer and nested widget (widget > container > widget)
src/widget.js
Outdated
| for ( const value of range ) { | ||
| const node = value.item; | ||
|
|
||
| if ( node.is( 'element' ) && isWidget( node ) ) { |
There was a problem hiding this comment.
As I commented, this code was just an ugly quick fix. I didn't mean the fact that it was defined in that listener – moving this code to a method doesn't change much (in fact, I'd revert this change – less code, less documentation). What I meant by "it needs refactoring" is that I added a nested if() for no good reason – you can flatten this and extract the condition to a helper function. Also, I think we could use the last element from previouslySelected if it was an array (although, I have mixed feeling about such change). Plus, the added condition requires a comment because Array.from( node.getAncestors() ).includes( lastMarked ) is hardly idiomatic.
|
Ready to review once again. |
src/utils.js
Outdated
| * @param {module:engine/view/element~Element|null} parent A parent for the element. | ||
| * @returns {Boolean} | ||
| */ | ||
| export function isChild( element, parent ) { |
There was a problem hiding this comment.
This util makes no sense in this place. This file contains widget utils. You added a general one.
See also https://github.com/ckeditor/ckeditor5-engine/issues/1588.
Please make this a private util of src/widget.js and add a comment under https://github.com/ckeditor/ckeditor5-engine/issues/1588 that it should be fixed.
|
@Reinmar, fixed. |
Suggested merge commit message (convention)
Fix: Selection converter will mark nested widgets properly. Closes ckeditor/ckeditor5#4594.