Heading plugin typescript definitions: support user-defined models?

[jake-netflix]

📝 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:

https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/src/headingconfig.ts#L75

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).

https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/src/headingconfig.ts#L82

https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/src/headingconfig.ts#L110

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:

https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/tests/headingediting.js#L93-L106

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):

<heading level="1">My heading</heading>
<heading level="2">My smaller heading</heading>

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.

[Witoso]

Thanks for the detailed request, we will discuss this internally, and get back to you.

[Witoso]

Bugs extracted to: #15790.

As for the no. 3:

I can't use the heading plugin because each option requires its own model name.

Could you elaborate a bit on why is that a problem?

[jake-netflix]

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. heading1, heading2, for the plugin to properly convert between model & views.

The heading plugin registers your configured model name in the schema, and an elementToElement converter:

https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/src/headingediting.ts#L71-L76

When executing the heading command, it currently changes the model name as the sole distinguisher between different heading levels: https://github.com/ckeditor/ckeditor5/blob/89c183174fe22cf0ec76dda530dc4b344ad36c93/packages/ckeditor5-heading/src/headingcommand.ts#L78

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 <heading> model, but it would be a nice QoL feature for it to be configuration-driven. Example conversion:

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!

[Mati365]

@jake-netflix

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?

[jake-netflix]

That is definitely helpful. The naming convention would be that you prefix custom headings w/ with heading-?

The callout I have is that CKEditor models are usually camelCase while the view is kebab-case. Not sure how strict of a requirement that is.

[Mati365]

@jake-netflix You are right. I relaxed a bit model name type and now camelCase, kebab-case, snake_case are allowed. Can you take a look again?

https://github.com/ckeditor/ckeditor5/pull/15833/files

[jake-netflix]

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 myCoolAmazingGiantText and it's still valid. I wonder if the naming convention might confuse new users of CkEditor, but that's just my 2 cents.

[Mati365]

@jake-netflix It introduces a breaking change in typing. The introduction of mode: string makes the union of HeadingElementOption | HeadingParagraphOption unnecessary, because the types are very similar (they differ by the presence of an optional attribute) and TypeScript merges them. Those who rely on these types after the editor update will have to rework their code. For now, it is better to keep changes as small as it is possible, despite being slightly less flexible.

[jake-netflix]

@Mati365 That makes total sense, thanks for the explanation! What you have is helpful for my use case already (and hopefully for others too).