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

#6432: Introduced SchemaItemDefinition#isSelectable and SchemaItemDefinition#isContent. #7770

Merged
merged 17 commits into from
Aug 5, 2020

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 4, 2020

Suggested merge commit message (convention)

Feature (engine): Introduced the SchemaItemDefinition#isSelectable property. Closes #6432.

Feature (engine): Introduced the SchemaItemDefinition#isContent property. Closes #7631.

Other (table): The tableCell model element brought by the TableEditing plugin is no longer an object (SchemaItemDefinition#isObject) in the Schema but a selectable (SchemaItemDefinition#isSelectable) (see #6432).

Internal (ui): Aligned the BalloonToolbar plugin behavior to the new  SchemaItemDefinition#isSelectable property (see #6432).

Docs (engine): Extended the "Schema" deep dive guide with the new properties and methods (see #6432, #7631).

Tests (image): Aligned tests to the fact that the tableCell model element is no longer an object but a selectable in the schema (see #6432).

Tests (horizontal-line): Aligned tests to the fact that the tableCell model element is no longer an object but a selectable in the schema (see #6432).

MINOR BREAKING CHANGE (table): The tableCell model element brought by the TableEditing plugin is no longer an object (SchemaItemDefinition#isObject) in the Schema but a selectable (SchemaItemDefinition#isSelectable). Please update your integration code accordingly (see #6432).


Additional information

@oleq oleq requested a review from Reinmar August 4, 2020 09:16
@oleq oleq marked this pull request as ready for review August 4, 2020 09:16
@oleq
Copy link
Member Author

oleq commented Aug 4, 2020

F2F call summary:

https://github.com/ckeditor/ckeditor5/blob/i/6432-schema-is-selectable/packages/ckeditor5-engine/src/model/utils/insertcontent.js#L243-L244 -> isSelectable()?

waiting for confirmation from @Reinmar 

https://github.com/ckeditor/ckeditor5/blob/i/6432-schema-is-selectable/packages/ckeditor5-engine/src/model/utils/modifyselection.js#L108 -> isSelectable()?

Yes.

https://github.com/ckeditor/ckeditor5/blob/i/6432-schema-is-selectable/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js#L181-L184 -> isSelectable()?

Yes.

https://github.com/ckeditor/ckeditor5/blob/i/6432-schema-is-selectable/packages/ckeditor5-horizontal-line/src/horizontallinecommand.js#L97 -> isSelectable()?
On one hand, it makes sense because it would disable the command when a single table cell is selected (enabled ATM in this PR).
OTOH, what if in the future we allow selecting some containers from the outside and converting them into a horizontal line when they are selected makes perfectly sense?

No.

@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2020

https://github.com/ckeditor/ckeditor5/blob/i/6432-schema-is-selectable/packages/ckeditor5-engine/src/model/utils/insertcontent.js#L243-L244 -> isSelectable()?

waiting for confirmation from @Reinmar 

This one is tricky. We actually want to check if it's:

  • a content, as if it's not content itself it could be ignored and only its children inserted in that place – as in _handleDisallowedNode,
  • a limit, as it also defines that the element cannot be split (or otherwise ignored),
  • selectable, asc we'll most likely try to select it.

Therefore, the current isObject() check is good. WITH ONE condition – that isObject() works this way:

isObject( item ) {
    const def = this.getDefinition( item );
    
    return def.isObject || ( def.isLimit && def.isSelectable && def.isContent );
}

You may remember that I already mentioned that isObject() should also return true if you set the 3 granular options to true. It'd definitely be safer in this case if we did that.

@oleq
Copy link
Member Author

oleq commented Aug 4, 2020

Let's have another round of review then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review Schema element properties Should tableCell be defined as object in schema?
2 participants