-
-
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
Heading plugin typescript definitions: support user-defined models? #15785
Comments
Thanks for the detailed request, we will discuss this internally, and get back to you. |
Bugs extracted to: #15790. As for the no. 3:
Could you elaborate a bit on why is that a problem? |
For number 3, it would probably entail a larger change for the heading plugin so I recognize it may not be addressable in the near-term, but thought I'd provide some feedback on this constraint for you guys to consider in the future. For my use case, I'm looking to have all the headings under one model, where the level is driven by an attribute. For example: <heading level="1">This is my large heading</heading>
<heading level="2">Slightly smaller heading</heading> This is the sort of thing that CkEditor's architecture facilitates quite well. However, with heading options config, you have to specify a unique model name, i.e. The heading plugin registers your configured model name in the schema, and an ckeditor5/packages/ckeditor5-heading/src/headingediting.ts Lines 71 to 76 in 89c1831
When executing the heading command, it currently changes the model name as the sole distinguisher between different heading levels:
So in order to leverage the existing heading plugin (i.e. its well-tested UI and commands), and allow each heading level to be its own dropdown menu item, each one needs to be a distinct model name. To work with this, I can still override conversions so that my output result has a normalized for (let headingOption of headingOptions) {
// Allow heading to show up as <heading level="2"> in the data for example.
this.editor.conversion.for('dataDowncast').elementToElement({
model: headingOption.model,
view: (_, {writer}) => {
return writer.createContainerElement('heading', {
[HEADING_LEVEL_ATTRIBUTE_NAME]: headingOption.level,
});
},
converterPriority: 'highest',
});
// Convert data back to its appropriate model.
this.editor.conversion.for('upcast').elementToElement({
model: headingOption.model,
view: {
name: 'heading',
attributes: [
{
key: HEADING_LEVEL_ATTRIBUTE_NAME,
value: headingOption.level,
},
],
},
converterPriority: 'highest',
});
} Thanks for your consideration! |
I have taken a look at the first two reported points and propose these changes: https://github.com/ckeditor/ckeditor5/pull/15833/files We would like to keep the changes to a minimum, while preserving type unions, which unfortunately necessitates a naming convention in the naming of heading models. Will the indicated changes solve your issue? |
That is definitely helpful. The naming convention would be that you prefix custom headings w/ with The callout I have is that CKEditor models are usually |
@jake-netflix You are right. I relaxed a bit model name type and now |
Just checked it out. That's a creative solution, and it's definitely helpful. To humor one point of discussion, do you think it's actually needed to constrain the model name at all? Since there's no technical limitation preventing a model name from being any string? For example, someone could define a model called |
@jake-netflix It introduces a breaking change in typing. The introduction of |
@Mati365 That makes total sense, thanks for the explanation! What you have is helpful for my use case already (and hopefully for others too). |
📝 Provide a description of the improvement
Part 1: Type definitions to support custom heading models
Currently the typescript definition for heading configs, uses this union type:
ckeditor5/packages/ckeditor5-heading/src/headingconfig.ts
Line 75 in 89c1831
These options explicitly specify heading1-6, and paragraph models. However, according to the docs, we're able to specify our own models. (Docs example is
headingFancy
).ckeditor5/packages/ckeditor5-heading/src/headingconfig.ts
Line 82 in 89c1831
ckeditor5/packages/ckeditor5-heading/src/headingconfig.ts
Line 110 in 89c1831
Could we add some
HeadingCustomOption
type to the union so we don't have to use@ts-ignore
?Part 2: UpcastAlso in typescript definitions?
I believe the heading options support
upcastAlso
as a property. For example in heading tests:ckeditor5/packages/ckeditor5-heading/tests/headingediting.js
Lines 93 to 106 in 89c1831
Can this be added to the typescript definitions for heading options?
Part 3: Allow attributes in model config?
If I wanted a model structure like this (not view):
I can't use the heading plugin because each option requires its own model name.
Ideally we could configure a model name with attribute value(s) similar to
ElementObjectDefinition
in the conversion pipeline.If you'd like to see this improvement implemented, add a 👍 reaction to this post.
The text was updated successfully, but these errors were encountered: