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

downcastElementToWidget? #1228

Closed
nickmanning214 opened this issue Aug 30, 2018 · 7 comments
Closed

downcastElementToWidget? #1228

nickmanning214 opened this issue Aug 30, 2018 · 7 comments
Labels
package:engine package:widget resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@nickmanning214
Copy link

nickmanning214 commented Aug 30, 2018

🆕 Feature request

I notice that the docs allude to downcastElementToWidget (link). This is kind of what I'm looking for I think. However, I can't find where this function actually comes from.

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

Hm... we may need to make it clearer. Such a function does not exist. It's theoretical. It was meant to show that you can use any function there, but it does it rather poorly. I'll rewrite that bit.

Regarding element => widget. There's toWidget() helper that you can use like in this manual test: ckeditor/ckeditor5-widget@4b04dc6

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

However, a downcastElementToWidget() helper kinda makes sense. It could simplify this code a bit because you would use downcastElementToElement() for dataDowncast and downcastElementToWidget() in editingDowncast. In both cases you'd be able to use the declarative form (no need to define the callback).

ckeditor5-widget might expose such function, so I'm confirming this ticket.

@Reinmar Reinmar added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). status:confirmed labels Sep 4, 2018
@Reinmar Reinmar added this to the backlog milestone Sep 4, 2018
Reinmar added a commit to ckeditor/ckeditor5-engine that referenced this issue Sep 4, 2018
scofalik pushed a commit to ckeditor/ckeditor5-engine that referenced this issue Sep 4, 2018
@Reinmar
Copy link
Member

Reinmar commented Jan 15, 2019

Besides downcastElementToWiget() would be nice, but it made me think that in many cases the only reason why you won't be able to use a simple two-way elementToElement() converter is that for that single data downcast you need the toWidget() functionality. I wonder...

  1. Could we introduce a two-way elementToWidget() converter? Would it be confusing?

  2. Perhaps elementToElement() could accept onDataDowncast and onEditingDowncast callbacks where you could do simple:

    elementToElement( {
    	model: 'simpleBox',
    	view: {
    		name: 'section',
    		classes: 'simple-box'
    	},
    	onEditingDowncast: ( element, writer ) => {
    		return toWidget( element, writer );
    	}
    } );

RFC (cc @pjasiun, @scofalik, @jodator, @oleq)

@Reinmar Reinmar modified the milestones: backlog, next Jan 15, 2019
@Reinmar
Copy link
Member

Reinmar commented Jan 15, 2019

PS. I'm rising the priority of this issue because it's really unjustified that you need to switch from elementToElement() to 3 separate converters when you want to define a widget.

@jodator
Copy link
Contributor

jodator commented Jan 16, 2019

I notice that the docs allude to downcastElementToWidget (link).

This issue is a bit old thus the link provided does not contains downcastElementToWidget() no more. AFAIR I've removed that part of docs while exposing the conversion utilities.

As for the proposition currently you'd have to:

const config = {
	model: 'simpleBox',
	view: {
		name: 'section',
		classes: 'simple-box'
	};

conversion.for( 'upcast' ).elementToElement( config );
conversion.for( 'dataDowncast' ).elementToElement( config )
conversion.for( 'editingDowncast' ).elementToElement( {
    model: 'simpleBox',
    view: ( element, writer ) => {
        const section = writer.createContainerElement( 'section', { class: 'simple-box' } );
        return toWidget( section, writer, { label: 'some widget' } );
    }
} );

// or probably should work also:

conversion.elementToElement( config );
conversion.for( 'editingDowncast' ).elementToElement( {
    model: 'simpleBox',
    view: ( element, writer ) => {
        const section = writer.createContainerElement( 'section', { class: 'simple-box' } );
        return toWidget( section, writer, { label: 'some widget' } );
    },
    converterPriority: 'high'
} );

a bit of bloat I see but I don't think that I like complexity of the proposal:

conversion.elementToElement( {
	model: 'simpleBox',
	view: {
		name: 'section',
		classes: 'simple-box'
	},
	onEditingDowncast: ( element, writer ) => {
		const section = writer.createContainerElement( 'section', { class: 'simple-box' } );
		return toWidget( section, writer, { label: 'some widget' } );
	}
} );

The placement / construct of the onEditingDowncast looks too simple in order to solve this one issue. I think that it rather should:

  1. extend the 'view' config:

    conversion.elementToElement( {
    	model: 'simpleBox',
    	view: {
    		name: 'section',
    		classes: 'simple-box',
    		onEditingDowncast: {
    			view: ( element, writer ) => {
    				const section = writer.createContainerElement(
    					'section',
    					{ class: 'simple-box' }
    				);
    				return toWidget( section, writer, { label: 'some widget' } );
    			},
    			converterPirority: 'high' // or other properties
    		}
    	}
    } );
  2. define what to change for given conversion pipeline pipeline:

    conversion.elementToElement( {
    	model: 'simpleBox',
    	view: {
    		name: 'section',
    		classes: 'simple-box'
    	},
    	// It overrides the definition for editingDowncast pipeline so either:
    	// - only changed configuration as below
    	// - or full config (with model definition, priority, etc) - but looks like not needed.
    	onEditingDowncast: {
    		view: ( element, writer ) => {
    			const section = writer.createContainerElement( 'section', { class: 'simple-box' } );
    			return toWidget( section, writer, { label: 'some widget' } );
    		}
    	}
    } );

Given the above I don't think that simple general two-way converters should work for every case. They should cover most of the cases but not all. The knowledge behind knowing why I need adding the onEditingDowncast is the same as with using separate one-way converters. Also IMO using separate one-way converters for this purpose (widgets) also draws clear separation. This separation isn't so obvious in this simple examples but might come as the complexity of widget handling grows.

About complexity: even the "simplest" widget (Media Embed) does way more in the converter callback that used examples and I don't see that packed into one converion.elementToElement() object:

https://github.com/ckeditor/ckeditor5-media-embed/blob/2e62743b99c1a32dc93a5c29148bb3e6a57ff45c/src/mediaembedediting.js#L171-L199

ps.: The onEditingDowncast would require to parse and construct that from convert group name editingDowncast - minor issue.


TL;DR: I think we're fine with current API and I wouldn't introduce complexity into simple general conversion helper. Also the long-awaited docs for conversion would help (a guide).

@jodator
Copy link
Contributor

jodator commented Oct 16, 2020

So we changed our (my) mind about this and we will probably work on exposing toWidget() helper.

You can track progress here: #7823. I'm also closing this in favor of #7823.

@jodator jodator closed this as completed Oct 16, 2020
@jodator jodator added package:widget package:engine resolution:duplicate This issue is a duplicate of another issue and was merged into it. labels Oct 16, 2020
@jodator jodator removed this from the nice-to-have milestone Oct 16, 2020
@ajssd
Copy link

ajssd commented Jan 27, 2022

There is still a reference to this Issue in the Documentation, as well as a suggestion to up-vote this (closed) issue.. That link, as well as the comment, should be updated with the current state of things.

As you can see, the code became much more verbose and far longer. This is because you used lower-level converters. We plan to provide more handy widget conversion utilities in the future. Read more (and 👍) in this ticket.

https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-a-block-widget.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine package:widget resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

5 participants