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

Custom heading dropdown menu #651

Closed
pat123456 opened this issue Nov 6, 2017 · 20 comments · Fixed by ckeditor/ckeditor5-heading#92
Closed

Custom heading dropdown menu #651

pat123456 opened this issue Nov 6, 2017 · 20 comments · Fixed by ckeditor/ckeditor5-heading#92
Labels
package:heading type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@pat123456
Copy link

pat123456 commented Nov 6, 2017

Hi,

I'm trying to add a custom heading dropdown menu in the toolbar of ckeditor 5 (something like "block/inline styles menu" in ckeditor 4).
In this menu there will be for example these items : "News title", "News paragraph", "Interview Title", "Interview paragraph", "Interview citation", etc...
and it will create html element with class attribute :
<h1 class="news">lorem</h1>, <h1 class="interview">ipsum</h1>, <p class="interview">lorem</p>, etc...

I've "cloned" the heading plug-in, and with the help of this : #481
i've modified headingengine.js like that :

    import ViewContainerElement from '../../ckeditor5-engine/src/view/containerelement';
    ...
    editor.config.define( 'heading', {
        options: [
            { modelElement: 'headingNews', viewElement: 'h1', title: 'News Heading', class: 'ck-heading_heading2', viewClass: 'news' },
            { modelElement: 'headingInterview', viewElement: 'h1', title: 'Interview Heading', class: 'ck-heading_heading2', viewClass: 'interview' }
        ]
    } );
    ...
    // Build converter from model to view for data and editing pipelines.
    buildModelConverter().for( data.modelToView, editing.modelToView )
        .fromElement( option.modelElement )
        // .toElement( option.viewElement );
        .toElement( data => {
            return new ViewContainerElement( option.viewElement, {
                class: option.viewClass
            } );
        } );

when I select a menu item it work well and create the corresponding html element with the correct class name, and when deselect the heading text and reselect it again the menu change consequently.

but when there is already html content in the editor or when paste this content on it :

    <div id="editor">
        <h1 class="news">Lorem</h1>
        <h1 class="interview">Ipsum</h1>
    </div>

the second h1 (in this example) get class="news" attribute

What need to be changed to work correctly ?
May be some thing not far from in the #481 example ?

    buildViewConverter().for( data.viewToModel )
        .fromElement( 'span' )
        .consuming( { attribute: [ 'color' ] } )
        .toAttribute( viewElement => {
            return { key: 'textColor', value: viewElement.getStyle( 'color' ) }
        } );

Sorry my bad english, hope this understandable

@pat123456 pat123456 changed the title custum heading dropdown menu Custom heading dropdown menu Nov 6, 2017
@jodator jodator added the type:question This issue asks a question (how to...). label Nov 7, 2017
@jodator
Copy link
Contributor

jodator commented Nov 7, 2017

It looks like you only have to change that view-to-model conversion. You'd like to define a conversion from element to element with custom creator function that returns model's element. Something like this:

buildViewConverter().for( data.viewToModel )
    .fromElement( 'h1' )
    .toElement( viewElement => {
        const classNames = [ ...viewElement.getClassNames() ];
        
        const headingType = getHeadingTypeFromClassNames( classNames );

        // headingType should be one of defined heading elements like 'headingNews'.
        return new ModelElement( headingType );
    } );

Obviously you need to implement getHeadingTypeFromClassNames method here.

ps.: If you have any questions in future we're also active on Stack Overflow (on ckeditor5 tag).

@pat123456
Copy link
Author

Thank you for your answer, it work great,
and sorry if I didn't ask in the right place, will ask in Stack Overflow next time.

Just an another little question about that :
if I put an alert(headingType) in the .toElement function, the alert is fired 2 times for each element, why ?

@jodator
Copy link
Contributor

jodator commented Nov 7, 2017

if I put an alert(headingType) in the .toElement function, the alert is fired 2 times for each element, why ?

I've checked similar thing in original ckeditor5-heading plugin and the .toElement() is called only once per converted element (on loading data).

// Build converter from view to model for data pipeline.
buildViewConverter().for( data.viewToModel )
	.fromElement( option.viewElement )
	.toElement( viewElement => {
		console.log('converting', viewElement);
		return new ModelElement( option.modelElement );
	} );

I'd check if you don't have buildViewConverter().for( data.viewToModel ) doubled but it's just a guess...

@pat123456
Copy link
Author

no I didn't doubled buildViewConverter()...

it come from the options array, if I put other elements like that :

    options: [
        { modelElement: 'headingInterview', viewElement: 'h1', title: 'Interview Heading', class: 'ck-heading_heading2', viewClass: 'interview' },
        { modelElement: 'headingNews', viewElement: 'h1', title: 'News Heading', class: 'ck-heading_heading2', viewClass: 'news' },
        { modelElement: 'headingVideo', viewElement: 'h2', title: 'Video Heading', class: 'ck-heading_heading3', viewClass: 'video' },
        { modelElement: 'headingPov', viewElement: 'h3', title: 'Pov Heading', class: 'ck-heading_heading4', viewClass: 'pov' }
    ]

the problem is just about h1 because there are two h1 in the array. But may be it's normal behavior ?

@scofalik
Copy link
Contributor

scofalik commented Nov 7, 2017

Would it be possible tor you to post all the custom code you have in a jsfiddle? It's hard to guess without seeing all the code together.

@pat123456
Copy link
Author

Hi,

here it is : https://jsfiddle.net/2tk84ypg/1/

@jodator
Copy link
Contributor

jodator commented Nov 7, 2017

OK, it's clearer now. The alert is called twice since you add two converters in loop for h1 heading.

You should remove buildViewConverter().for( data.viewToModel )... outside for ( const option of options ) { loop and for each heading type you should build separate converter.

Something like (but probably would need some optimizations):

for ( const option of options ) {
    // Schema. (probably this part needs some optimization too - like own loop
    // with only unque elements to not defnie h1 twice).
    editor.document.schema.registerItem( option.modelElement, '$block' );

    // Build converter from model to view for data and editing pipelines.
    buildModelConverter().for( data.modelToView, editing.modelToView )
    // ... rest of code
}

// Now build each header element once:
buildViewConverter().for( data.viewToModel )
    .fromElement( 'h1' )
    // the rest of logic for converting h1 with different classes

buildViewConverter().for( data.viewToModel )
    .fromElement( 'h2' )
    // the rest of logic for converting h2 with different classes

So the reason why alert is twice is because you have two h1 elements in configuration so you'd created the h1 element converted twice in the loop.

@pat123456
Copy link
Author

ok thanks !

i've upadted the code, but it still fired twice, may be I didn't well understood how to write the "//rest of logic" as you though ?
https://jsfiddle.net/2tk84ypg/2/

@pat123456
Copy link
Author

ps : have removed the buildViewConverter().for( data.viewToModel ) that I left by error in the loop, but it's still the same result
https://jsfiddle.net/2tk84ypg/3/

@jodator
Copy link
Contributor

jodator commented Nov 7, 2017

I've checked and it seems to be OK, console.logs:
selection_001
and HTML in a fidle:

<h2 class="video">Lorem</h2>
<h1 class="interview">Impsum</h1>
<h1 class="news">Dolor</h1>
<h3 class="pov">Sit amet</h3>
<h3 class="pov">Consectetur</h3>

Notice two pov logs for two h3.pov - so it's OK.

@pat123456
Copy link
Author

pat123456 commented Nov 7, 2017

Sorry... it was just a cache problem

I will try to optimize it now.

Thank you very much for your help. And thank a lot to all ckeditor 5 devs to your work, I very enjoy the way it go !

Have a good day

@pat123456
Copy link
Author

pat123456 commented Nov 7, 2017

a little optimization : https://jsfiddle.net/2tk84ypg/4/

need again to allow "p" and multi class

@pat123456
Copy link
Author

pat123456 commented Nov 14, 2017

Hi,
In order to allow paragraph with custom class the same way as I did for headings, I had to change the paragraph plugin code like that (same thing that I did in headingengine.js) :
Replace this in paragraph.js :

    buildViewConverter().for( data.viewToModel )
        .fromElement( 'p' )
        .toElement( 'paragraph' );

with that :

    buildViewConverter().for( data.viewToModel )
        .fromElement( 'p' )
        .toElement( viewElement => {
            const classNames = [ ...viewElement.getClassNames() ];
		        
            if(classNames.length > 0){
                const headingType = getHeadingTypeFromClassNames( classNames )
                        return new ModelElement( headingType );
                }
            } );
            var options=[{ modelElement: 'paragraphInterview', viewElement: 'p', title: 'Interview paragraph', class: 'ck-heading_heading2', viewClass: 'pinterview' }]

            function getHeadingTypeFromClassNames(pClassNames){
                let headingType;
                for ( const option of options ) {
                    if(option.viewClass == pClassNames[0]){
                        headingType = option.modelElement;
                    }
                }
                return headingType;
            }

It seems to work (don't know if there is side effect), but the problem is that it need to put the "options" array in two place : in the heading plugin (or in the create function of ckeditor) and in the paragraph plugin.

Anyway to make it nicer ?

@Reinmar
Copy link
Member

Reinmar commented Nov 14, 2017

Conversion, like most things in the editor, is event-based. And listeners have priorities. So, conceptually speaking, you should be able to use the default Paragraph plugin and write another feature (a clone of Heading) which will plug its converters before Paragraph's. These new converters should only handle specific cases and skip the rest, allowing the Paragraph's converter to do its job.

@scofalik could you create an example how to do that with buildViewConverter()?

@pat123456
Copy link
Author

pat123456 commented Nov 14, 2017

Thank you. It is what I thought, but when I did a try like that, by simply add a custom "p" in the options array in headingengine.js (without changing anything in paragraph.js plugin) :
https://jsfiddle.net/2tk84ypg/6/

The buildViewConverter().for( data.viewToModel ) is working, but the paragraph plugin seem to have priority on it and overwrite it (if I disabled the buidViewConverter() method in the paragraph.js plugin it work well)

I'll appreciate an example

@pat123456
Copy link
Author

pat123456 commented Nov 14, 2017

I've added "withPriority"

    .fromElement( element )
    .withPriority(1)

https://jsfiddle.net/2tk84ypg/8/

And it seem to work
Is it sufficient, and is it what you thought about ?

@Reinmar
Copy link
Member

Reinmar commented Nov 15, 2017

Yep, if it works, then that would be it. I thought that you may need to use a more complicated notation with .consuming() and stuff, but I never worked with this API (yet :D).

@pat123456
Copy link
Author

Yes it seems to work but don't know if there could be side effects... wait and see. Anyway, I'll stay with that until I'll know v.5 more in depth...
Thank you very much

@pat123456
Copy link
Author

pat123456 commented Nov 16, 2017

some small improvements
https://jsfiddle.net/2tk84ypg/13/

  • allow to have element with no "viewClass" in options array (like heading1, heading2... in original plugin)
  • if an element is pasted or already present in the editor and have a class name that is not present in options array (viewClass), or no class defined, it will be converted to heading1, 2...
  • but if an option have "default=true" parameter and his viewElement parameter = the html tag of the pasted one, it will be converted with this option viewClass name

@Reinmar
Copy link
Member

Reinmar commented Nov 27, 2017

In #638 (comment) I proposed a config format which would allow everyone to easily configure which elements should be handled/rendered by which heading option.

@Reinmar Reinmar added this to the iteration 14 milestone Jan 16, 2018
@Reinmar Reinmar added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). status:confirmed package:heading and removed type:question This issue asks a question (how to...). labels Jan 16, 2018
Reinmar added a commit to ckeditor/ckeditor5-heading that referenced this issue Jan 16, 2018
Enhancement: Added support for `ConverterDefinition` in the heading options configuration. Closes #72. Closes ckeditor/ckeditor5#651.

BREAKING CHANGE: `config.heading.options` format has changed. The valid `HeadingOption` syntax is now compatible with `ConverterDefinition`: `{ model: 'heading1', view: 'h2', title: 'Heading 1' }`.
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
4 participants