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

Fix errors in image plugin when removing htmlPreview from dialog definition #83

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Axinet
Copy link
Contributor

Axinet commented Feb 22, 2014

Add possibility to remove 'htmlPreview' uiElement from image plugin - fix errors.

CKEDITOR.document.getById( imagePreviewLoaderId ).setStyle( 'display', 'none' );
var loader = CKEDITOR.document.getById( imagePreviewLoaderId );
if ( loader )
CKEDITOR.document.getById( imagePreviewLoaderId ).setStyle( 'display', 'none' );

This comment has been minimized.

@mlewand

mlewand May 8, 2014

Member
  1. You might reuse loader variable here, rather than calling CKEDITOR.document.getById( imagePreviewLoaderId ) once again.

  2. You might also combine var loader variable initialisation with var original few lines above, so you will end up with:

    var original = this.originalElement,
        loader = CKEDITOR.document.getById( imagePreviewLoaderId );
    
@@ -267,7 +269,10 @@
link = element && editor.elementPath( element ).contains( 'a', 1 );

//Hide loader.
CKEDITOR.document.getById( imagePreviewLoaderId ).setStyle( 'display', 'none' );
var loader = CKEDITOR.document.getById( imagePreviewLoaderId );

This comment has been minimized.

@mlewand

mlewand May 8, 2014

Member

Similarly here - you can combine var loader and add it right after link initialization.

previewPreloader.setAttribute( 'src', newUrl );
dialog.preview.setAttribute( 'src', previewPreloader.$.src );
updatePreview( dialog );
if(dialog.preview){

This comment has been minimized.

@mlewand

mlewand May 8, 2014

Member

Try to keep consistent coding style, we use space in if statements, like so:

if( dialog.preview ) {
@mlewand

This comment has been minimized.

Copy link
Member

mlewand commented May 8, 2014

Thanks for the Pull Request!

Generally PR looks good, I've tested your changes and it works correctly. There are only very minor things to be changed in your patch, I've pointed them in code comments.

But still I would like to ask you for the Use Case ( so essentially the code ) which you use to remove htmlPreview from image dialog.

@Axinet

This comment has been minimized.

Copy link
Contributor Author

Axinet commented May 15, 2014

Thanks for review. I was really busy last time, but I'm going to introduce provided suggestions this weekend.

And about the Use Case: I would like to have really simple dialog for image includes in comments module I'm trying to write for (amateur cyclist's) team page. Below are code and image (dialog in polish locale, contains only url, alternative text and url on second tab).

plugins/simpleimage/plugin.js file

CKEDITOR.plugins.add('simpleimage', {
    requires: 'image',

    init: function(editor) {

        CKEDITOR.on( 'dialogDefinition', function( ev ) {
            // Take the dialog name and its definition from the event data.
            var dialogName = ev.data.name;
            var dialogDefinition = ev.data.definition;

            // Check if the definition is from the dialog window you are interested in (the "Image" dialog window).
            if ( dialogName === 'image' ) {
                dialogDefinition.removeContents( 'advanced' );

                var infoTab = dialogDefinition.getContents( 'info' );
                infoTab.remove('basic');
                infoTab.remove('htmlPreview');

                dialogDefinition.minHeight = '130';
            }
        });
    }
}); 

simpleimage

@Axinet

This comment has been minimized.

Copy link
Contributor Author

Axinet commented Aug 29, 2014

Proposed fixes had been made long time ago. I don't know if there are any notifications about commits in pull request, so maybe I should have written some comment about it, but I'm still inexperienced in pull request workflow.

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Sep 1, 2014

Hey. Sorry for that, but I missed your resubmitted PR. I'll review it now.

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Sep 1, 2014

I merged your PR into master with b01618f. Thanks!

@Reinmar Reinmar closed this Sep 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.