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

The UX of resizing small images. #9173

Merged
merged 12 commits into from
Mar 16, 2021
9 changes: 6 additions & 3 deletions packages/ckeditor5-image/theme/image.css
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
/* Make sure there is some space between the content and the image. Center image by default. */
margin: 1em auto;

/* Make sure the caption will be displayed properly (See: https://github.com/ckeditor/ckeditor5/issues/1870). */
min-width: 50px;

& img {
/* Prevent unnecessary margins caused by line-height (see #44). */
display: block;
Expand All @@ -21,9 +24,6 @@

/* Make sure the image never exceeds the size of the parent container (ckeditor/ckeditor5-ui#67). */
max-width: 100%;

/* Make sure the caption will be displayed properly (See: https://github.com/ckeditor/ckeditor5/issues/1870). */
min-width: 50px;
}
}

Expand Down Expand Up @@ -61,6 +61,9 @@
& .image > figcaption.ck-placeholder::before {
padding-left: inherit;
padding-right: inherit;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
}

/*
Expand Down
59 changes: 2 additions & 57 deletions packages/ckeditor5-widget/src/widgetresize/resizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* @module widget/widgetresize/resizer
*/

import View from '@ckeditor/ckeditor5-ui/src/view';
import Template from '@ckeditor/ckeditor5-ui/src/template';
import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
Expand All @@ -16,6 +15,7 @@ import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import mix from '@ckeditor/ckeditor5-utils/src/mix';

import ResizeState from './resizerstate';
import SizeView from './sizeview';

/**
* Represents a resizer for a single resizable object.
Expand All @@ -41,7 +41,7 @@ export default class Resizer {
*
* @protected
* @readonly
* @member {module:widget/widgetresize/resizer~SizeView} #_sizeUI
* @member {module:widget/widgetresize/sizeview} #_sizeUI
*/

/**
Expand Down Expand Up @@ -475,61 +475,6 @@ export default class Resizer {

mix( Resizer, ObservableMixin );

/**
* A view displaying the proposed new element size during the resizing.
*
* @extends {module:ui/view~View}
*/
class SizeView extends View {
constructor() {
super();

const bind = this.bindTemplate;

this.setTemplate( {
tag: 'div',
attributes: {
class: [
'ck',
'ck-size-view',
bind.to( 'activeHandlePosition', value => value ? `ck-orientation-${ value }` : '' )
],
style: {
display: bind.if( 'isVisible', 'none', visible => !visible )
}
},
children: [ {
text: bind.to( 'label' )
} ]
} );
}

bindToState( options, resizerState ) {
this.bind( 'isVisible' ).to( resizerState, 'proposedWidth', resizerState, 'proposedHeight', ( width, height ) =>
width !== null && height !== null );

this.bind( 'label' ).to(
resizerState, 'proposedHandleHostWidth',
resizerState, 'proposedHandleHostHeight',
resizerState, 'proposedWidthPercents',
( width, height, widthPercents ) => {
if ( options.unit === 'px' ) {
return `${ width }×${ height }`;
} else {
return `${ widthPercents }%`;
}
}
);

this.bind( 'activeHandlePosition' ).to( resizerState );
}

dismiss() {
this.unbind();
this.isVisible = false;
}
}

// @private
// @param {String} resizerPosition Expected resizer position like `"top-left"`, `"bottom-right"`.
// @returns {String} A prefixed HTML class name for the resizer element
Expand Down
80 changes: 80 additions & 0 deletions packages/ckeditor5-widget/src/widgetresize/sizeview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module widget/widgetresize/sizeview
*/

import View from '@ckeditor/ckeditor5-ui/src/view';

/**
* A view displaying the proposed new element size during the resizing.
*
* @extends {module:ui/view~View}
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
*/

magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
export default class SizeView extends View {
constructor() {
/**
* The position of the view defined based on the host size and active handle position.
*
* @private
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @readonly, no one will ever write to this property.

Copy link
Contributor Author

@magda-chrzescian magda-chrzescian Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not used in tests anymore, so I left it @Private.

Asserting viewPosition makes little sense here IMO because it is an internal component state for binding purposes only. The only thing that matters for this component is the class.

* @observable
* @member {String} #viewPosition
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
*/

super();
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved

const bind = this.bindTemplate;

this.setTemplate( {
tag: 'div',
attributes: {
class: [
'ck',
'ck-size-view',
bind.to( 'viewPosition', value => value ? `ck-orientation-${ value }` : '' )
],
style: {
display: bind.if( 'isVisible', 'none', visible => !visible )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs for #isVisible. Also, #_isVisible should also be protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not used in tests or outside the class actually, so I made it @Private.

}
},
children: [ {
text: bind.to( 'label' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing #label docs. This also looks like an internal state for template binding purposes, so let's make it @Protected and #_label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not used in tests or outside the class actually, so I made it @Private.

} ]
} );
}

bindToState( options, resizerState ) {
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
this.bind( 'isVisible' ).to( resizerState, 'proposedWidth', resizerState, 'proposedHeight', ( width, height ) =>
width !== null && height !== null );

this.bind( 'label' ).to(
resizerState, 'proposedHandleHostWidth',
resizerState, 'proposedHandleHostHeight',
resizerState, 'proposedWidthPercents',
( width, height, widthPercents ) => {
if ( options.unit === 'px' ) {
return `${ width }×${ height }`;
} else {
return `${ widthPercents }%`;
}
}
);

this.bind( 'viewPosition' ).to(
resizerState, 'activeHandlePosition',
resizerState, 'proposedHandleHostWidth',
resizerState, 'proposedHandleHostHeight',
// If the widget is too small to contain the size label, display the label above.
( position, width, height ) => width < 50 || height < 50 ? 'above-center' : position
);
}

dismiss() {
this.unbind();
this.isVisible = false;
}
}
56 changes: 56 additions & 0 deletions packages/ckeditor5-widget/tests/widgetresize/sizeview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import SizeView from '../../src/widgetresize/sizeview';
import ResizerState from '../../src/widgetresize/resizerstate';

describe( 'SizeView', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're testing this view properly now, then some essential tests are missing:

  • template structure assertion,
  • style.display tests,
  • children text (label) tests.

describe( 'activeHandlePosition', () => {
let sizeView, state;

beforeEach( async () => {
state = new ResizerState();
sizeView = new SizeView();

sizeView.bindToState( {}, state );
sizeView.render();
} );

it( 'should have valid `viewPosition` property and class if the widget width is smaller than 50px', () => {
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
state.activeHandlePosition = 'top-right';
state.update( {
handleHostWidth: 49,
handleHostHeight: 200
} );

expect( sizeView.viewPosition ).to.equal( 'above-center' );
expect( sizeView.element.classList.contains( 'ck-orientation-above-center' ) ).to.be.true;
} );

it( 'should have valid `viewPosition` property and class if the widget height is smaller than 50px', () => {
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
state.activeHandlePosition = 'top-right';
state.update( {
handleHostWidth: 200,
handleHostHeight: 49
} );

expect( sizeView.viewPosition ).to.equal( 'above-center' );
expect( sizeView.element.classList.contains( 'ck-orientation-above-center' ) ).to.be.true;
} );

it( 'should have valid `viewPosition` property and class if the widget width is bigger than 50px/50px', () => {
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
const position = 'top-right';

state.activeHandlePosition = position;
state.update( {
handleHostWidth: 200,
handleHostHeight: 200
} );

expect( sizeView.viewPosition ).to.equal( position );
expect( sizeView.element.classList.contains( `ck-orientation-${ position }` ) ).to.be.true;
} );
magda-chrzescian marked this conversation as resolved.
Show resolved Hide resolved
} );
} );
15 changes: 13 additions & 2 deletions packages/ckeditor5-widget/theme/widget.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

--ck-resizer-border-radius: var(--ck-border-radius);
--ck-resizer-tooltip-offset: 10px;
--ck-resizer-tooltip-height: calc(var(--ck-spacing-small) * 2 + 10px);
}

.ck .ck-widget {
Expand Down Expand Up @@ -51,12 +52,15 @@
border-radius: var(--ck-resizer-border-radius);
font-size: var(--ck-font-size-tiny);
display: block;
padding: var(--ck-spacing-small);
padding: 0 var(--ck-spacing-small);
height: var(--ck-resizer-tooltip-height);
line-height: var(--ck-resizer-tooltip-height);

&.ck-orientation-top-left,
&.ck-orientation-top-right,
&.ck-orientation-bottom-right,
&.ck-orientation-bottom-left {
&.ck-orientation-bottom-left,
&.ck-orientation-above-center {
position: absolute;
}

Expand All @@ -79,4 +83,11 @@
bottom: var(--ck-resizer-tooltip-offset);
left: var(--ck-resizer-tooltip-offset);
}

/* Class applied if the widget is too small to contain the size label */
&.ck-orientation-above-center {
top: calc(var(--ck-resizer-tooltip-height) * -1);
left: 50%;
transform: translate(-50%);
}
}