Opened 3 years ago

Closed 3 years ago

#13434 closed Bug (fixed)

Dialog busy state indicator is broken in RTL

Reported by: Olek Nowodziński Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.2
Component: UI : Dialogs Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by Olek Nowodziński)

It looks like no one tested the feature in RTL env. The position of the spinner is wrong. Implemented in #13213.

Attachments (1)

dialog-busy-rtl.png (51.5 KB) - added by Olek Nowodziński 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by Olek Nowodziński

Attachment: dialog-busy-rtl.png added

comment:1 Changed 3 years ago by Olek Nowodziński

Description: modified (diff)

comment:2 Changed 3 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:3 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:4 Changed 3 years ago by Tade0

Status: assignedreview

Changes pushed to branch:t/13434.

comment:5 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

Tests

  • Missing white space in **upper left corner**(**upper _right_ corner** https://github.com/cksource/ckeditor-dev/blame/be63bb8da05d53d1fac2baa6f2193cd94694bfff/tests/plugins/dialog/manual/state.md#L10
  • No loops are needed, instanceLoaded and CKEDITOR.replaceAll will do the trick:
    <textarea cols="80" name="en" class="ckeditor" rows="10"></textarea>
    ...
    <textarea cols="80" name="he" class="ckeditor" rows="10"></textarea>
    
    and
    CKEDITOR.on( 'instanceLoaded', function( evt ) {
    	CKEDITOR.dialog.add( 'state', function() {
    		return {
    			title: 'Dialog with state indicator',
    			...
    		};
    	} );
    } );
    
    CKEDITOR.replaceAll( function( el, config ) {
    	CKEDITOR.tools.extend( config, {
    		language: el.getAttribute( 'name' ),
    
    		on: {
    			pluginsLoaded: function() {
    				this.addCommand( 'state', new CKEDITOR.dialogCommand( 'state' ) );
    
    				this.addCommand( 'setRtl', {
    					exec: function( editor ) {
    					}
    				} );
    
    				this.ui.addButton( 'state', {
    					label: 'Show a dialog with state indicator',
    					command: 'state',
    				} );
    			}
    		}
    	} );
    } );
    

Code

  • Code–style (i.e. spinnerDesc.styles['float'] vs spinnerDesc.styles[ 'float' ] )
  • this.getParentEditor() instead of this._.editor
  • Save bytes. Instead of
    var spinnerDesc = {
    	attributes: {
    		'class': 'cke_dialog_spinner'
    	},
    	styles: {}
    };
    
    if ( this._.editor.lang.dir == 'rtl' ) {
    	spinnerDesc.styles['float'] = 'right';
    	spinnerDesc.styles['margin-left'] = '8px';
    } else {
    	spinnerDesc.styles['float'] = 'left';
    	spinnerDesc.styles['margin-right'] = '8px';
    }
    
    this.parts.spinner = CKEDITOR.document.createElement( 'div', spinnerDesc );
    
    you could do
    var dir = this.getParentEditor().lang.dir,
    	spinnerDef = {
    		attributes: {
    			'class': 'cke_dialog_spinner'
    		},
    		styles: {
    			float: dir == 'rtl' ? 'right' : 'left'
    		}
    	};
    
    spinnerDef.styles[ 'margin-' + ( dir == 'rtl' ? 'left' : 'right' ) ] = '8px';
    
    this.parts.spinner = CKEDITOR.document.createElement( 'div', spinnerDef );
    
    because things like styles['float'] or styles['margin-right'] will not be compressed in the building process.

comment:6 Changed 3 years ago by Tade0

Status: review_failedreview

Changes pushed to branch:t/13434.

comment:7 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:8 Changed 3 years ago by Olek Nowodziński

Pushed minor fixes to branch:t/13434 (HEAD at git:8178479). Waiting for master to switch from 4.4.8 to 4.5.1.

comment:9 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:7c8521b.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy