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

Consider simplifying attribute conversion #4063

Closed
Reinmar opened this issue May 11, 2017 · 6 comments
Closed

Consider simplifying attribute conversion #4063

Reinmar opened this issue May 11, 2017 · 6 comments
Labels
package:engine status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented May 11, 2017

Looking at the image feature converters I particularly didn't like this piece:

export function createImageAttributeConverter( dispatchers, attributeName ) {
	for ( let dispatcher of dispatchers ) {
		dispatcher.on( `addAttribute:${ attributeName }:image`, modelToViewAttributeConverter );
		dispatcher.on( `changeAttribute:${ attributeName }:image`, modelToViewAttributeConverter );
		dispatcher.on( `removeAttribute:${ attributeName }:image`, modelToViewAttributeConverter );
	}
}

function modelToViewAttributeConverter( evt, data, consumable, conversionApi ) {
	const parts = evt.name.split( ':' );
	const consumableType = parts[ 0 ] + ':' + parts[ 1 ];

	if ( !consumable.consume( data.item, consumableType ) ) {
		return;
	}

	const figure = conversionApi.mapper.toViewElement( data.item );
	const img = figure.getChild( 0 );

	if ( parts[ 0 ] == 'removeAttribute' ) {
		img.removeAttribute( data.attributeKey );
	} else {
		img.setAttribute( data.attributeKey, data.attributeNewValue );
	}
}

Couldn't we have just one event and the type of change as a data property? Could we simplify that consumable check code too? Maybe we could also have a shorthand method in Element – like updateAttribute() to avoid having to use both remove/setAttribute() methods.

@Reinmar
Copy link
Member Author

Reinmar commented May 11, 2017

@scofalik asked me:

Why converter builder has not been used?

cc @szymonkups – there was some reason AFAIR. But you're right that my ticket should be about improving the builder as well.

@szymonkups
Copy link
Contributor

szymonkups commented May 11, 2017

If I remember correctly, with builder we cannot specify that we want to convert attributes from certain element type only. For example: "convert src attribute only from image elements".

@scofalik
Copy link
Contributor

I can also see that there is figure involved in here, which looks like is mapped to the original data.item.

@szymonkups
Copy link
Contributor

I was mentioning this here: https://github.com/ckeditor/ckeditor5-engine/issues/736.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 11, 2017

@pjasiun
Copy link

pjasiun commented Feb 16, 2018

Fixed by ckeditor/ckeditor5-engine#1274 and other refactoring tickets.

@pjasiun pjasiun closed this as completed Feb 16, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added module:conversion status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants