t/152: Implemented FloatingToolbarView and Template.revert() #156

oleq · 2017-02-03T14:54:03Z

Proposed merge commit message

Feature: Implemented FloatingToolbarView, Implemented Template.revert(). Closes ckeditor/ckeditor5#5308.

TODO The above message may be outdated. Verify it!

…mented Template#revert()

…d as an argument.

…olbar utils.

Reinmar · 2017-02-06T21:40:41Z

src/toolbar/floating/floatingtoolbarview.js

+ *
+ * @extends module:ui/toolbar/toolbarview~ToolbarView
+ */
+export default class FloatingToolbarView extends ToolbarView {


I've got doubts about this class.

It fixes that it must be toolbar what floats and nothing more. What if we'd like to add one more element to it?

It fixes that the floating toolbar must be floating in this specific way. "Floating" is just a behaviour and we have at least two floating toolbars already.

This behaviour should rather be implemented similarly to the contextual toolbar – as a container for whatever you want to have inside. Just like the balloon.

…w styles instead.

Reinmar · 2017-03-02T10:14:42Z

src/editableui/editableuiview.js

+		 * @observable
+		 * @member {Boolean} #isRendered
+		 */
+		this.set( 'isRendered', !editableElement );


Kinda misleading name, considering the description. You can do better than that :P

Reinmar · 2017-03-02T10:17:48Z

src/panel/floating/floatingpanelview.js

+
+/**
+ * A default set of positioning functions used by the panel view to float
+ * around {@link targetElement}.


Incorrect link. Please run gulp docs and fix the errors.

Reinmar · 2017-03-02T10:18:39Z

src/panel/floating/floatingpanelview.js

+ *
+ * Positioning functions must be compatible with {@link module:utils/dom/position~Position}.
+ *
+ * @member {Object} module:ui/floatingpanel/floatingpanelview~FloatingPanelView#defaultPositions


This is a static property so .defaultPositions not #defaultPositions.

Also, don't link to protected properties from public ones. (_updatePosition).

Reinmar · 2017-03-02T10:21:18Z

src/template.js

+		 *
+		 * @protected
+		 * @readonly
+		 * @member {module:ui/template~RenderConfig#renderData}


Incorrect notation.

Reinmar · 2017-03-02T10:22:57Z

src/template.js

+	 * Reverts a template {@link module:ui/template~Template#apply applied} to an existing DOM Node.
+	 *
+	 * @param {Node} node Root node for the template to revert. Must be the same node
+	 * that {@link module:ui/template~Template#apply} has used.


Must be the same node?

Reinmar · 2017-03-02T10:23:12Z

src/template.js

+	 * that {@link module:ui/template~Template#apply} has used.
+	 */
+	revert( node ) {
+		if ( !node ) {


No point in checking this.

Reinmar · 2017-03-02T10:23:44Z

src/template.js

+			throw new CKEditorError( 'ui-template-revert-not-applied: Attempting reverting a template which has not been applied yet.' );
+		}
+
+		this._revertNode( node, this._revertData );


You do not revert a node. You remove the template from the node or revert template application from node.

Reinmar · 2017-03-02T10:26:10Z

src/template.js

 	 */
-	_renderNode( applyNode, intoFragment ) {
+	_renderNode( config ) {


We rather tend to call such state objects "data".

Reinmar · 2017-03-02T10:26:40Z

src/template.js

+/**
+ * The {@link module:ui/template~Template#_renderNode} config.
+ *
+ * @interface module:ui/template~RenderConfig


Isn't this private?

Reinmar · 2017-03-02T10:35:30Z

Not much to say here. I found some naming and docs issues. Other than that, I'm a bit worried about the Template size and amount of functionality which it brings. Fortunately, it's a simple (i.e. small) piece of API so there's little impact on the rest of the project if we'll need to work on the internals.

In general, are you happy with the stability of apply + remove? Or is it a piece we'll need to polish in the future?

…late#revert.

…rData.

oleq · 2017-03-07T16:17:29Z

I'm quite happy with how revert() works. It passes tests and works for the particular use case. Besides, it's not gonna be a piece of API which is used very often.

And, yes, Template will need to be split sooner or later into text renderer, element renderer, attribute renderer, etc. Template, much shorter than it is at this moment, would then only glue the pieces and give the nice API to the world.

Other: Fixed import paths after [refactoring in ckeditor5-ui](ckeditor/ckeditor5-ui#156). Closes #83.

Other: Fixed import paths after [refactoring in ckeditor5-ui](ckeditor/ckeditor5-ui#156). Closes #52.

Reinmar · 2017-03-07T18:20:02Z

I've just noticed that there's no manual test for the floating panel.

oleq · 2017-03-08T07:57:07Z

@Reinmar 👉 https://github.com/ckeditor/ckeditor5-ui/issues/161

oleq added 14 commits January 27, 2017 13:42

(Temp) Improved Template#apply() so it preserves original data. Imple…

624aa58

…mented Template#revert()

View#destroy() should not remove view's #element from DOM.

30e9455

Code refactoring and docs improvements in Template class.

98a0331

(TMP) Working draft of FloatingToolbarView.

c68588f

Docs: Fixes in Template class.

aa37943

Improved CC of Template class.

5b297ad

Template#apply() will extend only style and class attributes.

df62ec5

Core–refactoring, massive docs improvements.

703089a

[Tests, Docs, Code refac] for FloatingToolbarView.

1d5fd3e

Made EditableUIView revert its template if #editableElement was passe…

5f7f0a2

…d as an argument.

Implemented expandToolbarConfig() and enableToolbarKeyboardFocus() to…

23a029e

…olbar utils.

Added styles for FloatingToolbarView.

f38789a

Added styles for FloatingToolbarView. Code refac in *ToolbarView styles.

8d2b005

Merge branch 'master' into t/152

b1e6a3c

oleq mentioned this pull request Feb 3, 2017

t/1: Introduced Inline Editor ckeditor/ckeditor5-editor-inline#2

Merged

oleq changed the title ~~Feature: Implemented FloatingToolbarView, Implemented Template.revert(). Closes #152.~~ t/152: Implemented FloatingToolbarView and Template.revert() Feb 3, 2017

Reinmar reviewed Feb 6, 2017

View reviewed changes

Reinmar added the status:review- label Feb 6, 2017

oleq added 5 commits February 15, 2017 14:32

Renamed expandToolbarConfig->getItemsFromConfig.

efb8b58

Merge branch 'master' into t/152

d7050ee

Created FloatingPanelView class. Moved panels to panel/ directory.

eca5642

Added #content collection to FloatingPanelView.

9256a9a

Removed FloatingToolbarView from the codebase. Added FloatingPanelVie…

1311a72

…w styles instead.

oleq removed the status:review- label Feb 16, 2017

oleq added 2 commits March 1, 2017 12:00

Merge branch 'master' into t/152

191a983

Moved enableToolbarKeyboardFocus ToolbarView utils to a separate module.

0047487

oleq force-pushed the t/152 branch from 8643038 to 0047487 Compare March 1, 2017 11:13

Reinmar reviewed Mar 2, 2017

View reviewed changes

Reinmar added the status:review- label Mar 2, 2017

oleq added 3 commits March 7, 2017 12:04

Merge remote-tracking branch 'origin/master' into t/152

7b67c79

Renamed EditableUIView#isRendered to EditableUIView#externalElement.

d40c01b

Docs: Fixed broken links.

f3d5395

oleq force-pushed the t/152 branch from 65f8321 to f3d5395 Compare March 7, 2017 12:58

oleq added 5 commits March 7, 2017 14:00

Docs: Removed link to protected method from public API.

2349d63

Docs: Fixes in Template.

c7ba822

Removed unnecessary node presence checks from Template#apply and Temp…

35347df

…late#revert.

Renamed Template#_revertNode() into #_revertTemplateFromNode().

51830e9

Renamed module:ui/template~RenderConfig into module:ui/template~Rende…

e42cf46

…rData.

oleq removed the status:review- label Mar 7, 2017

Reinmar merged commit cb606d7 into master Mar 7, 2017

Reinmar deleted the t/152 branch March 7, 2017 17:59

Reinmar added a commit to ckeditor/ckeditor5-link that referenced this pull request Mar 7, 2017

Merge pull request #84 from ckeditor/t/83

b235415

Other: Fixed import paths after [refactoring in ckeditor5-ui](ckeditor/ckeditor5-ui#156). Closes #83.

Reinmar added a commit to ckeditor/ckeditor5-image that referenced this pull request Mar 7, 2017

Merge pull request #53 from ckeditor/t/52

cc8f671

Other: Fixed import paths after [refactoring in ckeditor5-ui](ckeditor/ckeditor5-ui#156). Closes #52.

oleq mentioned this pull request Oct 9, 2019

Implement manual test for FloatingPanelView ckeditor/ckeditor5#5313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t/152: Implemented FloatingToolbarView and Template.revert() #156

t/152: Implemented FloatingToolbarView and Template.revert() #156

oleq commented Feb 3, 2017 •

edited by pomek

Loading

Reinmar Feb 6, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar Mar 2, 2017

Reinmar commented Mar 2, 2017

oleq commented Mar 7, 2017 •

edited

Loading

Reinmar commented Mar 7, 2017

oleq commented Mar 8, 2017

t/152: Implemented FloatingToolbarView and Template.revert() #156

t/152: Implemented FloatingToolbarView and Template.revert() #156

Conversation

oleq commented Feb 3, 2017 • edited by pomek Loading

Proposed merge commit message

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Mar 2, 2017

oleq commented Mar 7, 2017 • edited Loading

Reinmar commented Mar 7, 2017

oleq commented Mar 8, 2017

oleq commented Feb 3, 2017 •

edited by pomek

Loading

oleq commented Mar 7, 2017 •

edited

Loading