Skip to content

Commit

Permalink
Code review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
illia-stv committed Oct 13, 2023
1 parent 2e28793 commit db8c25c
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 22 deletions.
19 changes: 14 additions & 5 deletions packages/ckeditor5-ckbox/src/ckboxediting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@ import {
type ViewElement,
type Writer
} from 'ckeditor5/src/engine';
import { CKEditorError, logError } from 'ckeditor5/src/utils';
import { CKEditorError, type DecoratedMethodEvent, logError } from 'ckeditor5/src/utils';

import type { CKBoxAssetDefinition } from './ckboxconfig';

import CKBoxCommand from './ckboxcommand';
import CKBoxUploadAdapter from './ckboxuploadadapter';
import type { ReplaceImageSourceCommand } from '@ckeditor/ckeditor5-image';

const DEFAULT_CKBOX_THEME_NAME = 'lark';

Expand Down Expand Up @@ -66,10 +67,18 @@ export default class CKBoxEditing extends Plugin {
const isLibraryLoaded = !!window.CKBox;

if ( replaceImageSourceCommand ) {
// After replacing image, "ckboxImageId" attribute will be removed.
replaceImageSourceCommand.registerImageCallback( ( writer, image ) => {
writer.removeAttribute( 'ckboxImageId', image );
} );
// // After replacing image, "ckboxImageId" attribute will be removed.
// replaceImageSourceCommand.registerImageCallback( ( writer, image ) => {
// writer.removeAttribute( 'ckboxImageId', image );
// } );

this.listenTo<DecoratedMethodEvent<ReplaceImageSourceCommand, 'cleanupImage'>>(
replaceImageSourceCommand,
'cleanupImage',
( _, [ writer, image ] ) => {
writer.removeAttribute( 'ckboxImageId', image );
}
);
}

// Proceed with plugin initialization only when the integrator intentionally wants to use it, i.e. when the `config.ckbox` exists or
Expand Down
19 changes: 3 additions & 16 deletions packages/ckeditor5-image/src/image/replaceimagesourcecommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@ import type { Writer, Element } from 'ckeditor5/src/engine';
export default class ReplaceImageSourceCommand extends Command {
declare public value: string | null;

declare private _imageCallbacks: Array<( writer: Writer, image: Element ) => void>;

constructor( editor: Editor ) {
super( editor );

this._imageCallbacks = [];
this.decorate( 'cleanupImage' );
}

/**
Expand All @@ -43,15 +41,6 @@ export default class ReplaceImageSourceCommand extends Command {
this.value = this.isEnabled ? element.getAttribute( 'src' ) as string : null;
}

/**
* Register callback which will be executed after command execution.
*
* @param callback Callback which will be called after command execution.
*/
public registerImageCallback( callback: ( writer: Writer, image: Element ) => void ): void {
this._imageCallbacks.push( callback );
}

/**
* Executes the command.
*
Expand All @@ -65,14 +54,14 @@ export default class ReplaceImageSourceCommand extends Command {
this.editor.model.change( writer => {
writer.setAttribute( 'src', options.source, image );

this._cleanupImage( writer, image );
this.cleanupImage( writer, image );
} );
}

/**
* Cleanup some image attributes.
*/
private _cleanupImage( writer: Writer, image: Element ) {
public cleanupImage( writer: Writer, image: Element ): void {
writer.removeAttribute( 'srcset', image );
writer.removeAttribute( 'sizes', image );

Expand All @@ -84,7 +73,5 @@ export default class ReplaceImageSourceCommand extends Command {
writer.removeAttribute( 'width', image );
writer.removeAttribute( 'height', image );
writer.removeAttribute( 'alt', image );

this._imageCallbacks.forEach( callback => callback( writer, image ) );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ describe( 'ReplaceImageSourceCommand', () => {
it( 'should clean up some attributes in responsive image', () => {
setModelData( model, `[<imageBlock
src="foo/bar.jpg"
ckboxImageId="id"
width="100"
height="200"
myCustomId="id"
alt="Example image"
sources="[{srcset:'url', sizes:'100vw, 1920px', type: 'image/webp'}]"
></imageBlock>]` );
Expand All @@ -62,14 +62,19 @@ describe( 'ReplaceImageSourceCommand', () => {
expect( element.getAttribute( 'width' ) ).to.equal( 100 );
expect( element.getAttribute( 'height' ) ).to.equal( 200 );
expect( element.getAttribute( 'alt' ) ).to.equal( 'Example image' );
expect( element.getAttribute( 'myCustomId' ) ).to.equal( 'id' );

command.on( 'cleanupImage', ( eventInfo, [ writer, image ] ) => {
writer.removeAttribute( 'myCustomId', image );
} );
command.execute( { source: 'bar/foo.jpg' } );

expect( element.getAttribute( 'src' ) ).to.equal( 'bar/foo.jpg' );
expect( element.getAttribute( 'sources' ) ).to.be.undefined;
expect( element.getAttribute( 'width' ) ).to.be.undefined;
expect( element.getAttribute( 'height' ) ).to.be.undefined;
expect( element.getAttribute( 'alt' ) ).to.be.undefined;
expect( element.getAttribute( 'myCustomId' ) ).to.be.undefined;
} );
} );

Expand Down

0 comments on commit db8c25c

Please sign in to comment.