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

Handle h1 even if editor is configured to support h2+ #2455

Closed
Reinmar opened this issue Mar 5, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-heading#114
Closed

Handle h1 even if editor is configured to support h2+ #2455

Reinmar opened this issue Mar 5, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-heading#114
Assignees
Labels
package:heading type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 5, 2018

I think we discussed it somewhere but I can't find it reported.

Basically, the idea is that, since the editor is by default configured to support h2+, when you paste a h1, it gets converted into a paragraph. A more useful behaviour would be if h1 was converted to h2 (and heading1 in the model, to be precise) at that point.

Also, when working on an answer to https://stackoverflow.com/questions/49052613/is-it-possible-to-attach-an-inline-editor-to-an-h1-in-ckeditor5 I realised that if we made this behaviour (h1 -> h2) available for setData() as well, there might be a new kind of confusion among developers. Suddenly, the h1 elements won't be dropped but they will become h2s.

@oskarwrobel
Copy link
Contributor

oskarwrobel commented Mar 5, 2018

I always forget that we start from h2 and I'm adding h1 to the content. When after conversion I'm seeing paragraph instead, my first thought is "I forgot about heading plugin"

@archonic
Copy link

archonic commented Jun 9, 2018

There's some debate about whether more than one H1 tag per document is valid HTML5, but let's say I wanted to allow it. I have this in my build config:

heading: {
  options: [
    { model: 'paragraph', title: 'Paragraph', class: 'ck-heading_paragraph' },
    { model: 'heading1', view: 'h1', title: 'Heading 1', class: 'ck-heading_heading1' },
    { model: 'heading2', view: 'h2', title: 'Heading 2', class: 'ck-heading_heading2' },
    { model: 'heading3', view: 'h3', title: 'Heading 3', class: 'ck-heading_heading3' },
    { model: 'heading4', view: 'h4', title: 'Heading 4', class: 'ck-heading_heading4' },
    { model: 'heading5', view: 'h5', title: 'Heading 5', class: 'ck-heading_heading5' },
    { model: 'heading6', view: 'h6', title: 'Heading 6', class: 'ck-heading_heading6' }
  ]
}

H1 elements are still being converted to p elements. Is that behaviour configurable?

@jodator
Copy link
Contributor

jodator commented Jun 9, 2018

@archonic Hi I've just checked your configuration on latest master and it works as it should:

ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [ Enter, Typing, Undo, Heading, Paragraph ],
		toolbar: [ 'heading', '|', 'undo', 'redo' ],
		heading: {
			options: [
				{ model: 'paragraph', title: 'Paragraph', class: 'ck-heading_paragraph' },
				{ model: 'heading1', view: 'h1', title: 'Heading 1', class: 'ck-heading_heading1' },
				{ model: 'heading2', view: 'h2', title: 'Heading 2', class: 'ck-heading_heading2' },
				{ model: 'heading3', view: 'h3', title: 'Heading 3', class: 'ck-heading_heading3' },
				{ model: 'heading4', view: 'h4', title: 'Heading 4', class: 'ck-heading_heading4' },
				{ model: 'heading5', view: 'h5', title: 'Heading 5', class: 'ck-heading_heading5' },
				{ model: 'heading6', view: 'h6', title: 'Heading 6', class: 'ck-heading_heading6' }
			]
		}
	} )
	.then( editor => {
		window.editor = editor;
	} )
	.catch( err => {
		console.error( err.stack );
	} );

and the DOM:
selection_004

Which version of the editor are you using? Maybe it's just some misconfiguration issue.

@archonic
Copy link

My bad, I must have been having a cache issue.

Am I right in thinking that configuring plugins is affecting a global whitelist? Do we have direct access to that whitelist in CKEditor 5?

@jodator
Copy link
Contributor

jodator commented Jun 11, 2018

Am I right in thinking that configuring plugins is affecting a global whitelist? Do we have direct access to that whitelist in CKEditor 5?

The configuration depends on a feature itself. Here with headings you have to configure which elements in model are mapped to the view. The plugin itself adds proper entries to the schema.

As for reading schema configuration I don't recall anything in particular so we might be missing something here. /cc @Reinmar - do we have something for reading whole schema?

@f1ames f1ames self-assigned this Oct 2, 2018
@f1ames
Copy link
Contributor

f1ames commented Oct 2, 2018

The proposed solution is to convert h1 to heading1 (which for default configuration results in h2). What about the cases when configuration is customized like:

heading: {
  options: [
    { model: 'heading1', view: 'h3', title: 'Heading 1', class: 'ck-heading_heading1' },
    { model: 'heading2', view: 'h4', title: 'Heading 2', class: 'ck-heading_heading2' },
    { model: 'heading3', view: 'h5', title: 'Heading 3', class: 'ck-heading_heading3' }
  ]
}

or

heading: {
  options: [
    { model: 'heading2', view: 'h2', title: 'Heading 2', class: 'ck-heading_heading2' },
    { model: 'heading3', view: 'h3', title: 'Heading 3', class: 'ck-heading_heading3' }
  ]
}

should h1 be converted to heading1 only if heading1 is available and mapped to h2, so { model: 'heading1', view: 'h2', title: 'Heading 1', class: 'ck-heading_heading1' } or should it use some more fancy algorithm to approach non-standard configurations? (First option seems reasonable and there is a lower risk of creating confusing results IMHO).

@Reinmar
Copy link
Member Author

Reinmar commented Oct 2, 2018

I think we can focus on this single scenario with <h1> not having a converter in the default configuration. It's enough.

Also, the second configuration is incorrect (options must start from heading1 – it's covered by the documentation).

Reinmar referenced this issue in ckeditor/ckeditor5-heading Oct 30, 2018
Other: The `<h1>` elements are now converted to `<heading1>` elements instead of being converted to `<paragraph>`s by default. Closes #98. Closes ckeditor/ckeditor5-paste-from-office#2.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-heading Oct 9, 2019
@mlewand mlewand added this to the iteration 21 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:heading labels Oct 9, 2019
@2-fly-4-ai
Copy link

Why would it convert h1 to h2? Am I missing something? I get that I should not have an h1 in my article's body, but this caught me off guard. Good to know I'm not going crazy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:heading 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.

7 participants