Skip to content
This repository has been archived by the owner on Nov 16, 2017. It is now read-only.

t/98: Extracted rect position utility from BalloonPanelView. #134

Merged
merged 9 commits into from
Dec 8, 2016
Merged

Conversation

oleq
Copy link
Member

@oleq oleq commented Dec 2, 2016

Requires #130.
Requires ckeditor/ckeditor5-utils#106.

Closes #98.

*/
attachTo( elementOrRange, limiterElementOrRect ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe we should move BalloonPanelView#limiter back to the argument of attachTo?

Copy link
Member

Choose a reason for hiding this comment

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

It depends on whether you'll be changing object to which the balloon is attached and the limiter. I think they go in pairs, so setting both at once makes more sense to me. If you'll decide to leave it separately then on the limiter change the balloon should reposition it self, of course. But that would also leave it unclear why the anchor is set through attachTo() and limitter through a setter.

Copy link
Member Author

Choose a reason for hiding this comment

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

*
* @type {Array.<String>}
*/
this.positions = [
Copy link
Member Author

Choose a reason for hiding this comment

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

Prototyping contextual toolbar I already learned that this is not the best way of configuration.

  1. First of all, a contextual toolbar must bring 3 new positions to BalloonPanelView.defaultPositions without affecting the prototype of the entire class. this.positions should rather contain actual functions than strings or BalloonPanelView.defaultPositions should become this.defaultPositions.
  2. Secondly, it turned out that positions may change a lot with each attachTo, like this
editor.listenTo( viewDocument, 'click', () => {
	if ( viewDocument.selection.isCollapsed ) {
		balloonPanelView.positions = [ 's' ];
	} else {
		balloonPanelView.positions =  [
			viewDocument.selection.isBackward ? 'backwardSelection' : 'forwardSelection'
		];
	}

	balloonPanelView.attachTo( viewDocument.domConverter.viewRangeToDom( viewDocument.selection.getFirstRange() ) );
} );

so probably attachTo should accept options object which would be merged with some default options, like

balloonPanelView.attachTo( {
	target: viewDocument.domConverter.viewRangeToDom( viewDocument.selection.getFirstRange() ),
	positions: [
		viewDocument.selection.isBackward ? 'backwardSelection' : 'forwardSelection'
	]
} );

mimicking getOptimalPosition API.

@@ -10,6 +10,7 @@ import ViewCollection from 'ckeditor5/ui/viewcollection.js';
import BalloonPanelView from 'ckeditor5/ui/balloonpanel/balloonpanelview.js';
import ButtonView from 'ckeditor5/ui/button/buttonview.js';
import testUtils from 'tests/core/_utils/utils.js';
import * as positionUtils from 'ckeditor5/utils/dom/position.js';
Copy link
Member

@Reinmar Reinmar Dec 7, 2016

Choose a reason for hiding this comment

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

Please don't use import *. That may (although, shouldn't) deoptimise tree shaking and it's unsafe because it cannot be well transpiled AFAIR (the module consumer has no knowledge about its content). I've just checked that we get this:

define(['exports', '../view.js', '../template.js', '../../utils/dom/position.js', '../../utils/dom/tounit.js'], function (exports, _view, _template, _position, _tounit) {
	'use strict';

	Object.defineProperty(exports, "__esModule", {
		value: true
	});

	var _view2 = _interopRequireDefault(_view);

	var _template2 = _interopRequireDefault(_template);

	var _tounit2 = _interopRequireDefault(_tounit);

	function _interopRequireDefault(obj) {
		/* istanbul ignore next */
		return obj && obj.__esModule ? obj : {
			default: obj
		};
	}

The position doesn't go through interop. But perhaps it's not a big deal...

Copy link
Member

Choose a reason for hiding this comment

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

Bleh, I've just noticed that these are tests :D So ignore my comment, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're welcome! ;-)

@Reinmar Reinmar merged commit f5d9a1c into master Dec 8, 2016
@Reinmar Reinmar deleted the t/98 branch December 8, 2016 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants