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

Autoformat for headings rely on the heading package too deeply #2373

Closed
Reinmar opened this issue Jun 8, 2017 · 4 comments · Fixed by ckeditor/ckeditor5-autoformat#30
Closed
Assignees
Labels
package:autoformat type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jun 8, 2017

The code looks like this now:

	_addHeadingAutoformats() {
		const commands = this.editor.commands;
		const options = this.editor.config.get( 'heading.options' );

		if ( options ) {
			for ( const option of options ) {
				const commandName = option.modelElement;
				let match;

				if ( commands.has( commandName ) && ( match = commandName.match( /\d+$/ ) ) ) {
					const level = match[ 0 ];
					const regExp = new RegExp( `^(#{${ level }})\\s$` );

					// eslint-disable-next-line no-new
					new BlockAutoformatEngine( this.editor, regExp, context => {
						const { batch } = context;

						this.editor.execute( commandName, { batch } );
					} );
				}
			}
		}
	}

What I don't understand is why is the autoformat dependent on the config option? Can't it simply depend on commands that it can find? You never know how config option will be turned into commands (if one didn't load the heading plugin there may be no commands despite the config option being set!).

@szymonkups
Copy link
Contributor

Heading commands' names are created based on configuration. Command name is same as modelElement from configuration. We need to find a way to match regexp with correct level. For example: how to know that ## should execute heading2 command?

I know this is not the best solution but I couldn't find better one.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 8, 2017

We should be looking for headingN in the command collection and just use the N to know the number of # which we look for. Wouldn't that be enough?

@szymonkups
Copy link
Contributor

We can go with that. It will just assume that all command names will be in form headingN but I think it's not a big deal.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 8, 2017

I think it's a safe assumption. Besides, the heading feature is configured with model->view mapping. So, no matter which heading levels you want to use in the view, you can (and should) always start from heading1 in the model. The autoformat feature can then base on that.

szymonkups referenced this issue in ckeditor/ckeditor5-autoformat Jun 8, 2017
Other: The autoformat feature will not depend on the configuration of the heading feature but it will use the available `heading*` commands. Closes #29.
@Reinmar Reinmar self-assigned this Jun 8, 2017
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-autoformat Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:autoformat labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:autoformat type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants