From f910622e978b210bb0cca5ce9da7574c76b485c7 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 24 Jul 2017 14:25:51 +0200 Subject: [PATCH 1/4] Implemented the Tooltip component. --- src/tooltip/tooltipview.js | 83 +++++++++++++++++++++++++++++++++++ tests/tooltip/tooltipview.js | 58 ++++++++++++++++++++++++ theme/components/tooltip.scss | 80 ++++++++++----------------------- 3 files changed, 164 insertions(+), 57 deletions(-) create mode 100644 src/tooltip/tooltipview.js create mode 100644 tests/tooltip/tooltipview.js diff --git a/src/tooltip/tooltipview.js b/src/tooltip/tooltipview.js new file mode 100644 index 00000000..e7d5bb28 --- /dev/null +++ b/src/tooltip/tooltipview.js @@ -0,0 +1,83 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module ui/tooltip/tooltipview + */ + +import View from '../view'; +import Template from '../template'; + +/** + * The tooltip view class. + * + * @extends module:ui/view~View + */ +export default class TooltipView extends View { + /** + * @inheritDoc + */ + constructor( locale ) { + super( locale ); + + /** + * The text of the tooltip visible to the user. + * + * @observable + * @member {String} #text + */ + this.set( 'text' ); + + /** + * The direction of the tooltip (south or north). + * + * +-----------+ + * | north | + * +-----------+ + * V + * [element] + * + * [element] + * ^ + * +-----------+ + * | south | + * +-----------+ + * + * @observable + * @default 's' + * @member {'s'|'n'} #position + */ + this.set( 'position', 's' ); + + const bind = this.bindTemplate; + + this.template = new Template( { + tag: 'span', + attributes: { + class: [ + 'ck-tooltip', + bind.to( 'position', position => 'ck-tooltip_' + position ) + ] + }, + children: [ + { + tag: 'span', + + attributes: { + class: [ + 'ck-tooltip__text' + ] + }, + + children: [ + { + text: bind.to( 'text' ), + } + ] + } + ] + } ); + } +} diff --git a/tests/tooltip/tooltipview.js b/tests/tooltip/tooltipview.js new file mode 100644 index 00000000..2a5c71dc --- /dev/null +++ b/tests/tooltip/tooltipview.js @@ -0,0 +1,58 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import TooltipView from '../../src/tooltip/tooltipview'; + +describe( 'TooltipView', () => { + let view, text; + + beforeEach( () => { + view = new TooltipView(); + text = view.element.firstChild; + } ); + + describe( 'constructor()', () => { + it( 'should create element from template', () => { + expect( view.element.tagName ).to.equal( 'SPAN' ); + expect( view.element.classList.contains( 'ck-tooltip' ) ).to.be.true; + expect( view.element.childNodes ).to.have.length( 1 ); + + expect( text.tagName ).to.equal( 'SPAN' ); + expect( text.classList.contains( 'ck-tooltip__text' ) ).to.be.true; + } ); + + it( 'should set default #position', () => { + expect( view.position ).to.equal( 's' ); + } ); + } ); + + describe( 'DOM bindings', () => { + beforeEach( () => { + view.text = 'foo'; + } ); + + describe( 'text content', () => { + it( 'should react on view#text', () => { + expect( text.textContent ).to.equal( 'foo' ); + + view.text = 'baz'; + + expect( text.textContent ).to.equal( 'baz' ); + } ); + } ); + + describe( 'class', () => { + it( 'should react on view#position', () => { + expect( view.element.classList.contains( 'ck-tooltip_n' ) ).to.be.false; + expect( view.element.classList.contains( 'ck-tooltip_s' ) ).to.be.true; + + view.position = 'n'; + + expect( view.element.classList.contains( 'ck-tooltip_n' ) ).to.be.true; + expect( view.element.classList.contains( 'ck-tooltip_s' ) ).to.be.false; + } ); + } ); + } ); +} ); diff --git a/theme/components/tooltip.scss b/theme/components/tooltip.scss index 26a91dff..fe7ac6d8 100644 --- a/theme/components/tooltip.scss +++ b/theme/components/tooltip.scss @@ -1,43 +1,12 @@ // Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. // For licensing, see LICENSE.md or http://ckeditor.com/license -/** - * Applies styles to the main part of the tooltip. - */ -@mixin ck-tooltip__main { - &::after { - @content; - } -} - -/** - * Applies styles to the arrow part of the tooltip. - */ -@mixin ck-tooltip__arrow { - &::before { - @content; - } -} - -/** - * Applies styles to both arrow and main part of the tooltip. - */ -@mixin ck-tooltip__elements { - @include ck-tooltip__main { - @content; - } - - @include ck-tooltip__arrow { - @content; - } -} - /** * Enables the tooltip, which is the tooltip is in DOM but * not yet displayed. */ @mixin ck-tooltip_enabled { - @include ck-tooltip__elements { + .ck-tooltip { display: block; } } @@ -46,7 +15,7 @@ * Disables the tooltip making it disappear from DOM. */ @mixin ck-tooltip_disabled { - @include ck-tooltip__elements { + .ck-tooltip { display: none; } } @@ -56,37 +25,34 @@ * Requires `ck-tooltip_enabled` first. */ @mixin ck-tooltip_visible { - @include ck-tooltip__elements { + .ck-tooltip { visibility: visible; opacity: 1; } } -[data-ck-tooltip] { - @include ck-tooltip__elements { - // Tooltip is hidden by default. - visibility: hidden; - opacity: 0; - display: none; - - position: absolute; - z-index: ck-z( 'modal' ); +.ck-tooltip, +.ck-tooltip__text::after { + position: absolute; - // Without this, hovering the tooltip could keep it visible. - pointer-events: none; + // Without this, hovering the tooltip could keep it visible. + pointer-events: none; - // This is to get rid of flickering when transitioning opacity in Chrome. - // It's weird but it works. - -webkit-backface-visibility: hidden; - } + // This is to get rid of flickering when transitioning opacity in Chrome. + // It's weird but it works. + -webkit-backface-visibility: hidden; +} - @include ck-tooltip__main { - content: attr(data-ck-tooltip); - } +.ck-tooltip { + // Tooltip is hidden by default. + visibility: hidden; + opacity: 0; + display: none; + z-index: ck-z( 'modal' ); +} - @include ck-tooltip__arrow { - content: ""; - width: 0; - height: 0; - } +.ck-tooltip__text::after { + content: ""; + width: 0; + height: 0; } From 246f13149c0ac355b18a8c7a9e72f49a580d1424 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 24 Jul 2017 14:28:17 +0200 Subject: [PATCH 2/4] Fix: Button tooltip should not look blurry on ldpi screens. Closes #142. Closes #133. --- src/button/buttonview.js | 29 ++++++++++++----- tests/button/buttonview.js | 65 +++++++++++++++++++++++++++++--------- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/src/button/buttonview.js b/src/button/buttonview.js index d026d28c..fd36520a 100644 --- a/src/button/buttonview.js +++ b/src/button/buttonview.js @@ -10,6 +10,7 @@ import View from '../view'; import Template from '../template'; import IconView from '../icon/iconview'; +import TooltipView from '../tooltip/tooltipview'; import { getEnvKeystrokeText } from '@ckeditor/ckeditor5-utils/src/keyboard'; @@ -140,6 +141,13 @@ export default class ButtonView extends View { * @member {module:ui/icon/iconview~IconView} #iconView */ + /** + * Tooltip of the button view. + * + * @readonly + * @member {module:ui/tooltip/tooltipview~TooltipView} #tooltipView + */ + const bind = this.bindTemplate; this.template = new Template( { @@ -148,16 +156,12 @@ export default class ButtonView extends View { attributes: { class: [ 'ck-button', - bind.if( '_tooltipString', 'ck-tooltip_s' ), bind.to( 'isEnabled', value => value ? 'ck-enabled' : 'ck-disabled' ), bind.if( 'isVisible', 'ck-hidden', value => !value ), bind.to( 'isOn', value => value ? 'ck-on' : 'ck-off' ), bind.if( 'withText', 'ck-button_with-text' ) ], type: bind.to( 'type', value => value ? value : 'button' ), - 'data-ck-tooltip': [ - bind.to( '_tooltipString' ) - ], tabindex: bind.to( 'tabindex' ) }, @@ -207,17 +211,26 @@ export default class ButtonView extends View { * @inheritDoc */ init() { - if ( this.icon && !this.iconView ) { + if ( this.icon ) { const iconView = this.iconView = new IconView(); iconView.bind( 'content' ).to( this, 'icon' ); - this.element.insertBefore( iconView.element, this.element.firstChild ); - // Make sure the icon view will be destroyed along with button. + // Make sure the icon will be destroyed along with the button. this.addChildren( iconView ); } + if ( this.tooltip ) { + const tooltipView = this.tooltipView = new TooltipView(); + + tooltipView.bind( 'text' ).to( this, '_tooltipString' ); + this.element.appendChild( tooltipView.element ); + + // Make sure the tooltip will be destroyed along with the button. + this.addChildren( tooltipView ); + } + super.init(); } @@ -229,7 +242,7 @@ export default class ButtonView extends View { } /** - * Gets value for the `data-ck-tooltip` attribute from the combination of + * Gets the text for the {@link #tooltipView} from the combination of * {@link #tooltip}, {@link #label} and {@link #keystroke} attributes. * * @private diff --git a/tests/button/buttonview.js b/tests/button/buttonview.js index ce09c11d..6424e3a7 100644 --- a/tests/button/buttonview.js +++ b/tests/button/buttonview.js @@ -8,6 +8,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import ButtonView from '../../src/button/buttonview'; import IconView from '../../src/icon/iconview'; +import TooltipView from '../../src/tooltip/tooltipview'; testUtils.createSinonSandbox(); @@ -75,29 +76,56 @@ describe( 'ButtonView', () => { } ); describe( 'tooltip', () => { + beforeEach( () => { + view = new ButtonView( locale ); + } ); + it( 'is not initially set', () => { - expect( view.element.dataset.ckTooltip ).to.undefined; + expect( view.tooltipView ).to.be.undefined; + expect( view.element.childNodes ).to.have.length( 1 ); } ); it( 'is not initially set (despite #label and #keystroke)', () => { view.label = 'foo'; view.keystroke = 'A'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.undefined; + expect( view.tooltipView ).to.be.undefined; } ); it( 'is not set if neither `true`, String or Function', () => { view.label = 'foo'; view.keystroke = 'A'; view.tooltip = false; + view.init(); - expect( view.element.dataset.ckTooltip ).to.undefined; + expect( view.tooltipView ).to.be.undefined; view.tooltip = 3; - expect( view.element.dataset.ckTooltip ).to.undefined; + expect( view.tooltipView ).to.be.undefined; view.tooltip = new Date(); - expect( view.element.dataset.ckTooltip ).to.undefined; + expect( view.tooltipView ).to.be.undefined; + } ); + + it( 'when set, is added to the DOM', () => { + view.tooltip = 'foo'; + view.icon = 'bar'; + view.init(); + + expect( view.element.childNodes ).to.have.length( 3 ); + expect( view.element.childNodes[ 2 ] ).to.equal( view.tooltipView.element ); + expect( view.tooltipView ).to.instanceOf( TooltipView ); + } ); + + it( 'when set, is destroyed along with the view', () => { + view.tooltip = 'foo'; + view.init(); + + const spy = sinon.spy( view.tooltipView, 'destroy' ); + + view.destroy(); + sinon.assert.calledOnce( spy ); } ); describe( 'defined as a Boolean', () => { @@ -105,21 +133,23 @@ describe( 'ButtonView', () => { view.tooltip = true; view.label = 'bar'; view.keystroke = 'A'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.equal( 'bar (A)' ); + expect( view.tooltipView.text ).to.equal( 'bar (A)' ); } ); it( 'reacts to changes in #label and #keystroke', () => { view.tooltip = true; view.label = 'foo'; view.keystroke = 'B'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.equal( 'foo (B)' ); + expect( view.tooltipView.text ).to.equal( 'foo (B)' ); view.label = 'baz'; view.keystroke = false; - expect( view.element.dataset.ckTooltip ).to.equal( 'baz' ); + expect( view.tooltipView.text ).to.equal( 'baz' ); } ); } ); @@ -128,16 +158,19 @@ describe( 'ButtonView', () => { view.tooltip = 'bar'; view.label = 'foo'; view.keystroke = 'A'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.equal( 'bar' ); + expect( view.tooltipView.text ).to.equal( 'bar' ); } ); it( 'reacts to changes of #tooltip', () => { view.tooltip = 'bar'; - expect( view.element.dataset.ckTooltip ).to.equal( 'bar' ); + view.init(); + + expect( view.tooltipView.text ).to.equal( 'bar' ); view.tooltip = 'foo'; - expect( view.element.dataset.ckTooltip ).to.equal( 'foo' ); + expect( view.tooltipView.text ).to.equal( 'foo' ); } ); } ); @@ -146,21 +179,23 @@ describe( 'ButtonView', () => { view.tooltip = ( l, k ) => `${ l } - ${ k }`; view.label = 'foo'; view.keystroke = 'A'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.equal( 'foo - A' ); + expect( view.tooltipView.text ).to.equal( 'foo - A' ); } ); it( 'reacts to changes of #label and #keystroke', () => { view.tooltip = ( l, k ) => `${ l } - ${ k }`; view.label = 'foo'; view.keystroke = 'A'; + view.init(); - expect( view.element.dataset.ckTooltip ).to.equal( 'foo - A' ); + expect( view.tooltipView.text ).to.equal( 'foo - A' ); view.label = 'bar'; view.keystroke = 'B'; - expect( view.element.dataset.ckTooltip ).to.equal( 'bar - B' ); + expect( view.tooltipView.text ).to.equal( 'bar - B' ); } ); } ); } ); @@ -218,7 +253,7 @@ describe( 'ButtonView', () => { describe( 'icon', () => { it( 'is not initially set', () => { expect( view.element.childNodes ).to.have.length( 1 ); - expect( view.iconView ).to.undefined; + expect( view.iconView ).to.be.undefined; } ); it( 'is set when view#icon is defined', () => { From d5da8a5197bbba5a510abb8bf8fba54fdff8d772 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 24 Jul 2017 14:56:39 +0200 Subject: [PATCH 3/4] Adjusted spacing of the tooltips. --- theme/components/tooltip.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/theme/components/tooltip.scss b/theme/components/tooltip.scss index fe7ac6d8..8f1a1022 100644 --- a/theme/components/tooltip.scss +++ b/theme/components/tooltip.scss @@ -51,6 +51,10 @@ z-index: ck-z( 'modal' ); } +.ck-tooltip__text { + display: inline-block; +} + .ck-tooltip__text::after { content: ""; width: 0; From e18ddf36998d2097d3b33417b03bfa003eafb1ac Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 25 Jul 2017 11:15:18 +0200 Subject: [PATCH 4/4] Implemented ButtonView#tooltipPosition attribute to control TooltipView#position. --- src/button/buttonview.js | 13 +++++++++++++ src/tooltip/tooltipview.js | 2 +- tests/button/buttonview.js | 13 +++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/button/buttonview.js b/src/button/buttonview.js index fd36520a..57ecb050 100644 --- a/src/button/buttonview.js +++ b/src/button/buttonview.js @@ -60,6 +60,18 @@ export default class ButtonView extends View { */ this.set( 'tooltip' ); + /** + * The position of the tooltip. See {@link ui/tooltip/tooltipview~TooltipView#position} + * to learn more about the available position values. + * + * **Note:** It makes sense only when the {@link #tooltip} is active. + * + * @observable + * @default 's' + * @member {'s'|'n'} #position + */ + this.set( 'tooltipPosition', 's' ); + /** * The HTML type of the button. Default `button`. * @@ -225,6 +237,7 @@ export default class ButtonView extends View { const tooltipView = this.tooltipView = new TooltipView(); tooltipView.bind( 'text' ).to( this, '_tooltipString' ); + tooltipView.bind( 'position' ).to( this, 'tooltipPosition' ); this.element.appendChild( tooltipView.element ); // Make sure the tooltip will be destroyed along with the button. diff --git a/src/tooltip/tooltipview.js b/src/tooltip/tooltipview.js index e7d5bb28..6cdf5075 100644 --- a/src/tooltip/tooltipview.js +++ b/src/tooltip/tooltipview.js @@ -31,7 +31,7 @@ export default class TooltipView extends View { this.set( 'text' ); /** - * The direction of the tooltip (south or north). + * The position of the tooltip (south or north). * * +-----------+ * | north | diff --git a/tests/button/buttonview.js b/tests/button/buttonview.js index 6424e3a7..8f8a6348 100644 --- a/tests/button/buttonview.js +++ b/tests/button/buttonview.js @@ -116,6 +116,7 @@ describe( 'ButtonView', () => { expect( view.element.childNodes ).to.have.length( 3 ); expect( view.element.childNodes[ 2 ] ).to.equal( view.tooltipView.element ); expect( view.tooltipView ).to.instanceOf( TooltipView ); + expect( view.tooltipView.position ).to.equal( 's' ); } ); it( 'when set, is destroyed along with the view', () => { @@ -128,6 +129,18 @@ describe( 'ButtonView', () => { sinon.assert.calledOnce( spy ); } ); + it( 'when set, reacts to #tooltipPosition attribute', () => { + view.tooltip = 'foo'; + view.icon = 'bar'; + view.init(); + + expect( view.tooltipPosition ).to.equal( 's' ); + expect( view.tooltipView.position ).to.equal( 's' ); + + view.tooltipPosition = 'n'; + expect( view.tooltipView.position ).to.equal( 'n' ); + } ); + describe( 'defined as a Boolean', () => { it( 'renders tooltip text out of #label and #keystroke', () => { view.tooltip = true;