Introduced ContextualBalloon class for managing contextual balloons. #177

oskarwrobel · 2017-03-28T18:31:48Z

Suggested merge commit message (convention)

Feature: Introduced ContextualBalloon plugin for managing contextual balloons. Closes ckeditor/ckeditor5#5296.

Follow-ups:

Improve ContextualBalloon positioning ckeditor5#5324 - Improve ContextualBalloon positioning.
Improve ContextualBalloon manual test ckeditor5#5330 - Improve ContextualBalloon manual test.
Custom action when panel of ContextualBalloon becomes visible ckeditor5#5326 - Custom action when panel of ContextualBalloon becomes visible.
Component to manage a stack of child views ckeditor5#5332 - Component to manage a stack of child views.

oskarwrobel · 2017-03-28T18:47:50Z

tests/manual/contextualballoon/contextualballoon.js

@@ -0,0 +1,283 @@
+/**


Code of this manual test is no pretty because a lot of various things happens there :)
This code should be reduced when ContextualToolbar will become a CKE5 plugin.

oleq · 2017-04-03T13:29:39Z

I see TypeError: Cannot read property 'add' of undefined error in http://localhost:8125/ckeditor5-ui/tests/manual/contextualballoon/contextualballoon.html, unable to play with manual test.

… -> Editor integration.

oleq · 2017-04-04T08:50:37Z

Not sure if this is a bug, but reporting anyway:

oskarwrobel · 2017-04-04T10:58:28Z

Hm... this is not a bug of ContextualBalloon class but manual test should be improved.

oleq · 2017-04-04T10:48:53Z

src/contextualballoon.js

+		/**
+		 * Balloon panel view.
+		 *
+		 * @member {module:ui:view~View} #view


@readonly?

Anyway, any practical reason why not creating the BalloonPanelView instance directly at this point but assigning it like ctxBalloon.view = new BalloonPanelView() later on instead?

I moved it to the constructor.

oleq · 2017-04-04T10:51:09Z

src/contextualballoon.js

+	 * @param {module:ui/contextualballoon~Panel} panelData Configuration of the panel.
+	 */
+	add( panelData ) {
+		if ( this._stack.get( panelData.view ) ) {


Wouldn't this.isPanelInStack( panelData.view ) be cleaner?

Yes. I added isPanelInstack() after I wrote this code.

oleq · 2017-04-04T10:51:51Z

src/contextualballoon.js

+			/**
+			 * Trying to add configuration of the same panel more than once.
+			 *
+			 * @error contextualballoon-add-item-exist


Error id in docs does not match error id in throw new CKEditorError().

oleq · 2017-04-04T10:53:00Z

src/contextualballoon.js

+		 * @private
+		 * @member {Map} #_stack
+		 */
+		this._stack = new Map();


@readonly?

Why not @private?

A good point :P

oleq · 2017-04-04T10:53:30Z

src/contextualballoon.js

+ * Common contextual balloon of the Editor.
+ *
+ * This class reuses the same {module:ui/view~View} for each contextual balloon panel in the editor UI, makes
+ * possible to add multiple panels to the same balloon (stored in the stack, last one in the stack is visible)


Why calling it "panels"? They're simply views, aren't they? It's a tool to use a single BalloonPanelView with multiple child views. The word "panel" repeats many times in this PR.

oleq · 2017-04-04T10:55:23Z

src/contextualballoon.js

+	 * @param {module:ui/view~View} view View of panel which will be removed from the balloon.
+	 */
+	remove( view ) {
+		if ( !this._stack.get( view ) ) {


this.isPanelInStack( view )

oleq · 2017-04-04T10:57:32Z

src/contextualballoon.js

+			this._stack.delete( view );
+
+			// Next we need to check if there is other panel in stack to show.
+			const lastPanel = Array.from( this._stack ).pop();


Using this._stack.values() would let us avoid lastPanel[ 1 ] a couple of lines later.

oleq · 2017-04-04T11:17:49Z

src/contextualballoon.js

+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+
+/**
+ * Common contextual balloon of the Editor.


How much work would it take to make it a generic utility? I suppose eventually we may end up with other UI components which should manage their children in a similar way. E.g. a dialog, where you edit some fields, then you go deeper to edit some details or internals, then you go back again, etc., without leaving the dialog. Each editing level would be a different child view of the same dialog. If this is a lot of thinking, designing and coding we may think about it as a follow–up, because it's not necessary to MVP.

I created follow-up #186

oleq · 2017-04-04T11:19:08Z

tests/manual/contextualballoon/contextualballoon.js

@@ -0,0 +1,292 @@
+/**


It would be great to see how to the focus behaves when navigating such structure.

Focus behave is a matter of feature implementation. In this PR ckeditor/ckeditor5-link#92 balloon uses focus cycler and works fine.

I created issue about simplifying/improving manual test. https://github.com/ckeditor/ckeditor5-ui/issues/185

oleq · 2017-04-05T11:43:13Z

src/contextualballoon.js

+	/**
+	 * Returns `true` when given view is in the stack otherwise returns `false`.
+	 *
+	 * @param {module:ui:view~View} view


Invalid link.

oleq · 2017-04-05T11:43:54Z

src/contextualballoon.js

+	}
+
+	/**
+	 * Adds view to the stack and makes is visible.


oleq · 2017-04-05T11:47:53Z

I also experienced

Uncaught TypeError: Cannot read property 'view' of null

when pressing Esc when just loaded http://localhost:8125/ckeditor5-ui/tests/manual/contextualballoon/contextualballoon.html.

oleq · 2017-04-05T12:08:17Z

src/contextualballoon.js

+ * This plugin allows reusing a single {module:ui/panel/balloon/balloonpanelview~BalloonPanelView} instance
+ * to display multiple contextual balloon panels in the editor.
+ *
+ * Child views of such a panel are stored in the stack and the last one in the stack is visible. When the


An afterthought: Since _stack is private we should not talk in the public docs about it. It is of no concern to the devs how the CB handles views.

When I wrote about stack I wasn't thinking about var name but about the way how we store it.

oleq · 2017-04-05T12:09:13Z

src/contextualballoon.js

+	 * @param {module:ui/view~View} view
+	 * @returns {Boolean}
+	 */
+	isViewInStack( view ) {


I would rename it to hasView(). As in https://github.com/ckeditor/ckeditor5-ui/pull/177/files#r109897538, _stack is private and it does not concern people in the public API.

oleq · 2017-04-05T12:32:36Z

src/contextualballoon.js

+	 *
+	 * @returns {module:ui/contextualballoon~ViewConfig|null}
+	 */
+	get visible() {


Boolean properties and variables are always prefixed by an auxiliary verb:

https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style-Naming-Guidelines

This getter returns visible view. It's not checking if given view is visible.

So get visibleView() maybe? I'm trying to make it less ambiguous.

Hm... this returns not only the view but the whole configuration { view, position } https://github.com/ckeditor/ckeditor5-ui/pull/177/files#diff-93d63500d90d723217fc16438b6b32a4R193

But maybe only the view should be returned.

Hm... looks like there is no need to return the balloon position configuration because this position is used internally by ContextualBalloon.

oleq · 2017-04-05T13:50:47Z

R+ but the master is locked so we're gonna postpone the merge.

oskarwrobel added 2 commits March 28, 2017 20:26

Introduced ContextualBalloon class for managing contextual balloons.

24b3784

Added manual test for ContextualBalloon.

66ed2ac

oskarwrobel commented Mar 28, 2017

View reviewed changes

oskarwrobel requested a review from oleq March 28, 2017 18:53

Improved docs.

3c7fb73

oskarwrobel force-pushed the t/134 branch from 880859b to 3c7fb73 Compare March 28, 2017 19:58

oskarwrobel mentioned this pull request Mar 28, 2017

Integrated ContextualBalloon. ckeditor/ckeditor5-editor-classic#56

Closed

oskarwrobel changed the title ~~Introduced ContextualToolbar class for managing contextual balloons.~~ Introduced ContextualBalloon class for managing contextual balloons. Mar 29, 2017

Added isPanelInStack method to the ContextualBalloon interface.

20804ee

oskarwrobel mentioned this pull request Mar 29, 2017

Integrated link UI with editor ContextualBalloon. ckeditor/ckeditor5-link#92

Merged

Fixed ContextualBalloon manual test to work without ContextualBalloon…

7b008e2

… -> Editor integration.

oskarwrobel mentioned this pull request Apr 4, 2017

T/182 Introduced contextual toolbar component #183

Closed

oleq suggested changes Apr 4, 2017

View reviewed changes

oleq added the status:review- label Apr 4, 2017

oskarwrobel added 3 commits April 5, 2017 08:15

Moved init of the balloon view to the class constructor.

1958b63

Renamed panel to view.

0ff3a52

Simplified the code.

28ba5b2

oskarwrobel removed the status:review- label Apr 5, 2017

oskarwrobel added 3 commits April 5, 2017 11:32

Changed ContextualBalloon from component to plugin.

67ee399

Aligned test to the lastes changes.

d4b1470

Fixed failing test.

3de51b3

oleq suggested changes Apr 5, 2017

View reviewed changes

oleq added the status:review- label Apr 5, 2017

oskarwrobel added 2 commits April 5, 2017 13:51

Improved docs.

24e5c60

Improved manual test.

a23df06

oskarwrobel removed the status:review- label Apr 5, 2017

Docs: Docs improvements in the ContextualBalloon plugin.

782cb5c

oleq reviewed Apr 5, 2017

View reviewed changes

Renamed isViewInStack to hasView.

c334894

oleq reviewed Apr 5, 2017

View reviewed changes

oskarwrobel added 2 commits April 5, 2017 15:21

Renamed and simplified visible getter.

dc1f532

Minor refactoring.

58d7ce9

oleq added the status:review+ label Apr 5, 2017

oskarwrobel merged commit 3ceb6a6 into master Apr 5, 2017

oskarwrobel deleted the t/134 branch April 5, 2017 18:32

oskarwrobel mentioned this pull request Oct 9, 2019

Component to manage a stack of child views ckeditor/ckeditor5#5332

Closed

Introduced ContextualBalloon class for managing contextual balloons. #177

Introduced ContextualBalloon class for managing contextual balloons. #177

Conversation

oskarwrobel commented Mar 28, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Follow-ups:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Apr 3, 2017

oleq commented Apr 4, 2017

oskarwrobel commented Apr 4, 2017

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

oskarwrobel Apr 4, 2017 • edited Loading

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

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

oleq commented Apr 5, 2017

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

oskarwrobel Apr 5, 2017 • edited Loading

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

oleq commented Apr 5, 2017

oskarwrobel commented Mar 28, 2017 •

edited by pomek

Loading

oskarwrobel Apr 4, 2017 •

edited

Loading

oskarwrobel Apr 5, 2017 •

edited

Loading