-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Modify SchemaContext #4250
Comments
Off topic: constructor( context ) {
if ( Array.isArray( context ) ) {
if ( context[ 0 ] && typeof context[ 0 ] != 'string' && context[ 0 ].is( 'documentFragment' ) ) {
context.shift();
}
}
else {
// `context` is item or position.
// Position#getAncestors() doesn't accept any parameters but it works just fine here.
context = context.getAncestors( { includeSelf: true } );
if ( context[ 0 ].is( 'documentFragment' ) ) {
context.shift();
}
}
this._items = context.map( mapContextItem );
} Why is the |
I'd go with the same instance. Since it's immutable anyway, that doesn't matter. |
We decided to call it |
Feature: Convert view to model using position. Closes #1213. Closes #1250. BREAKING CHANGE: `DataController#parse`, `DataController#toModel`, `ViewConversionDispatcher#convert` gets `SchemaContextDefinition` as a contex instead of `String`. BREAKING CHANGE: `ViewConversionApi#splitToAllowedParent` has been introduced. BREAKING CHANGE: `ViewConversionApi#storage` has been introduced. BREAKING CHANGE: `ViewConsumable` has been merged to `ViewConversionApi`. BREAKING CHANGE: Format od data object passed across conversion callback has been changed. Feature: `Schema#findAllowedParent` has been introduced. Feature: `SchemaContext#concat` has been introduced.
I'm afraid that I don't like the new method and other changes. I don't understand why we needed it.
EDIT: OK, that paragraph explains the "why". Sorry – I focused so much on Let's talk further under ckeditor/ckeditor5-engine#1263. |
At this moment, the schema is not mutable and there is no easy way to create one schema based on another. The simplest way right now is to create a new definition and the new schema based on it. Even the current docs do not look very simple:
https://github.com/ckeditor/ckeditor5-engine/blob/d00c24c0fefad47f6a48abe8bd43c71ee8fddf58/src/model/schema.js#L1018
You also, need to remember about all these fixes done in the
SchemaContext
creator if you want to get items from the position manually:https://github.com/ckeditor/ckeditor5-engine/blob/d00c24c0fefad47f6a48abe8bd43c71ee8fddf58/src/model/schema.js#L877-L890
This is why I think that schema
SchemaContext
should have methods likeadd
/delete
orpush
/pop
to create one context based one another. These methods could modify the context object, but if we want to keep a context immutable (as @Reinmar suggested) these methods could return the new context object (do not change the current one).Also,
checkChild
andcheckAttribute
should acceptSchemaContext
instance. To do so, we could makeSchemaContext
constructor acceptsSchemaContext
instance as a parameter. In such case, the same (or even the same instance) should be returned from the constructor.The text was updated successfully, but these errors were encountered: