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

List of allowed headings should be configurable #2418

Closed
pjasiun opened this issue Sep 21, 2016 · 9 comments · Fixed by ckeditor/ckeditor5-heading#57
Closed

List of allowed headings should be configurable #2418

pjasiun opened this issue Sep 21, 2016 · 9 comments · Fixed by ckeditor/ckeditor5-heading#57
Assignees
Labels
package:heading type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Sep 21, 2016

It should be configurable if one wants to have all headers (h2, h3 and h4), or only part of them.

@pjasiun pjasiun changed the title List of allowed heading should be configurable List of allowed headings should be configurable Sep 21, 2016
@fredck
Copy link
Contributor

fredck commented Sep 21, 2016

Actually, the configuration should set how deep headings nesting is allowed, defaulting to 3, up to 5 (or 6 if we also configure to start with h1).

This is not a requirement right now tough.

@pjasiun
Copy link
Author

pjasiun commented Sep 21, 2016

Actually, in my case, I only need h3 and h4. I want to be able to disable h2.

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2016

We simply need to expose this in the config.heading.<something>.

Also, I don't like the name of that property (which then repeates in the headings command API) because it's derived from the CKE4's format feature. So it should not be "formats" but... "headings"? :P

PS. I'm going to rename this feature today, so it's not called "headings" finally (it should be "heading").

@fredck
Copy link
Contributor

fredck commented Sep 21, 2016

PS. I'm going to rename this feature today, so it's not called "headings" finally (it should be "heading").

Hum... I don't think this is to be changed: https://github.com/ckeditor/ckeditor5-headings/issues/8#issuecomment-223593035

@oleq
Copy link
Member

oleq commented Sep 21, 2016

We simply need to expose this in the config.heading..

@Reinmar: How would this work with locales? Because strings like 'Paragraph', 'Heading 1', etc. must be translated. How could people access our localization tools in such config?

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2016

Hum... I don't think this is to be changed: #2408 (comment)

Hm... so I'm a bit confused, TBH, cause I thought for the whole time that you prefer singular. I'm even pretty sure that you convince me to rename this feature :D. I'll report a separate ticket for that or comment on #2408.

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2016

@Reinmar: How would this work with locales? Because strings like 'Paragraph', 'Heading 1', etc. must be translated. How could people access our localization tools in such config?

It's tricky, but we had an idea for that. Basically, we can provide localization only for the default options. The "localisator", while building the editor, will scan for t() usage and should find all default names like "Paragraph" and "Heading1" (so we must put somewhere explicit t('Paragraph') expressions in the code). It will put them as translatable tokens into the translation service.

Now, if someone defined config.heading while initialising the editor, they couldn't use t() there (because there's no editor yet) so we must treat those names as translation ids, not the final strings. The t() function will look for translation and if it couldn't find one, it will use the passed string. Which means that if you configured editor like this:

headings: [
  { name: 'Paragraph', ... }
  { name: 'Super heading', ... }
}

And used Polish locale, then you'll have the following options: "Akapit", "Super heading". So basically, it's your fault that only half of the texts were translated. You need to take care of passing the right strings.

But, if you're just using a subset of the default settings (which contains "Paragraph" and "Heading 1-3") like this:

headings: [
  { name: 'Paragraph', ... }
  { name: 'Heading 1', ... }
}

Then it will work perfectly even in Polish locale, because all strings will be translated automatically (cause we treat the setting as a translation ids).

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2016

@fredck: https://github.com/ckeditor/ckeditor5-headings/issues/6 there's a ticket for that. And the comment is later than the one in #2408. So we changed the opinion :D.

@fredck
Copy link
Contributor

fredck commented Sep 21, 2016

Well, I don't see much discussion there, still "heading" sounds weird.

@oleq oleq self-assigned this Feb 16, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-heading Mar 1, 2017
Feature: Enabled configuration and localization of available headings (see `config.heading.options`). Closes #33.

BREAKING CHANGES: The `heading` command now accepts `id` option, not `formatId`.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-heading Oct 9, 2019
@mlewand mlewand added this to the iteration 8 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:heading labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:heading type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants