Introduce the widget toolbar repository #54

ma2ciek · 2018-09-18T16:58:04Z

Suggested merge commit message (convention)

Feature: Introduced the widget toolbar repository. Closes ckeditor/ckeditor5#5486.

Additional information

After that change the following PRs can be merged:

coveralls · 2018-09-18T17:16:08Z

Coverage remained the same at 100.0% when pulling 031e370 on t/ckeditor5-ui/442 into 74ed902 on master.

pjasiun · 2018-09-19T09:56:06Z

src/widgettoolbar.js

+ *			}
+ *		}
+ */
+export default class WidgetToolbar extends Plugin {


Agree with @Reinmar that WidgetToolbarRepository will be better.

pjasiun · 2018-09-19T11:35:39Z

src/widgettoolbar.js

+import BalloonPanelView from '@ckeditor/ckeditor5-ui/src/panel/balloon/balloonpanelview';
+import { isWidget } from './utils';
+
+const defaultBalloonClassName = 'ck-toolbar-container';


I would not define it as here. Just use a string in the method, especially since it is used once. KISS.

Reinmar

whenVisible sounds wrong. Like: "do this something when the toolbar is visible". If anything it should be visibleWhen (like we discussed in https://github.com/ckeditor/ckeditor5-ui/issues/442#issuecomment-422719810) or showWhen.

pjasiun · 2018-09-19T12:34:33Z

src/widgettoolbarrepository.js

+	 *
+	 * @param {String} toolbarId Toolbar identificator.
+	 */
+	deregister( toolbarId ) {


It should be deregister or unregister? Also, I think we do not need this method.

I was thinking about removing the toolbar on the destroy() method. But it's questionable.

I also think we could remove these methods (deregister() and isRegistered()) for now. Unless any of them are used somewhere.

ma2ciek · 2018-09-19T12:41:01Z

whenVisible sounds wrong. Like: "do this something when the toolbar is visible". If anything it should be visibleWhen (like we discussed in ckeditor/ckeditor5-ui#442 (comment)) or showWhen.

Right, my bad. I reverted it to the correct form 👍

Reinmar · 2018-09-19T12:46:42Z

tests/widgettoolbarrepository.js

+		} );
+	} );
+
+	describe( 'deregister', () => {


deregister()

Reinmar · 2018-09-19T12:46:52Z

tests/widgettoolbarrepository.js

+		} );
+	} );
+
+	describe( 'isRegistered', () => {


isRegistered()

pjasiun · 2018-09-19T12:59:50Z

src/widgettoolbarrepository.js

+				view: toolbar.view,
+				position: getBalloonPositionData( this.editor ),
+				balloonClassName: toolbar.balloonClassName,
+			} );


I am not sure, but if the toolbar was already added to the balloon, shouldn't we just move it on top instead of adding it again? //cc @oleq

I see that contextual balloon throw when you add the same view twice (https://github.com/ckeditor/ckeditor5-ui/blob/85920b547c5d20ab4fcc1d495b19498c23a692d8/src/panel/balloon/contextualballoon.js#L126). I think it will be saver to remove the view first in such case (if the view was already added but is not on the top) and then re-add it.

So that's why there is else if ( !this._balloon.hasView( toolbar.view ) )

But maybe I don't see smth.

So that's why there is else if ( !this._balloon.hasView( toolbar.view ) )

It means that if the view was already added but is not on top nothing happen (including no error). Is it correct?

AFAIK yes 😄

We should reposition or show the view only if it's the most top view in the stack.

I've added a description of this method.

pjasiun · 2018-09-19T13:20:17Z

src/widgettoolbarrepository.js

+ *
+ *				widgetToolbarRepository.add( {
+ *					toolbarItems: editor.config.get( 'image.toolbar' )
+ *					visibleWhen: isImageWidgetSelected


Need to be updated.

Reinmar · 2018-09-19T13:35:50Z

src/widgettoolbarrepository.js

+ *				const editor = this.editor;
+ *				const widgetToolbarRepository = editor.plugins.get( WidgetToolbarRepository );
+ *
+ *				widgetToolbarRepository.register( {


Missing toolbar name.

Reinmar · 2018-09-19T13:38:07Z

src/widgettoolbarrepository.js

+	 * `visibleWhen` function. Toolbar items are gathered from `toolbarItems` array.
+	 * The balloon's CSS class is by default `ck-toolbar-container` and may be override with the `balloonClassName` option.
+	 *
+	 * Note: This method should be called in the {@link module:core/plugin/Plugin~afterInit} to make sure that plugins for toolbar items


Note: This method should be called in the {@link module:core/plugin/Plugin#afterInit `Plugin#afterInit()`} callback (or later) to make sure that the given toolbar items were already registered by other plugins.

Reinmar · 2018-09-19T13:42:11Z

tests/widgettoolbarrepository.js

+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+
+describe( 'WidgetToolbar', () => {


Reinmar

Also, I miss a link to this plugin in docs/api/widget.md.

Besides the above mentioned core functionalities, this package implements the following utils:

* The {@link module:widget/widgettoolbarrepository~WidgetToolbarRepository `WidgetToolbarRepository`} plugin which exposes a nice API for registering widget toolbars.
* A couple of helper functions for managing widgets in the {@link module:widget/utils `@ckeditor/ckeditor5-widget/utils`} module.

Reinmar · 2018-09-19T13:43:23Z

src/widgettoolbarrepository.js

+
+/**
+ * Widget toolbar repository plugin. A central point for creating widget toolbars. This plugin handles the whole
+ * toolbar rendering process and exposes concise API.


Widget toolbar repository plugin. A central point for registering widget toolbars. This plugin handles the whole toolbar rendering process and exposes a concise API. To add a toolbar for your widget use the {@link module:widget/widgettoolbarrepository~WidgetToolbarRepository#register `WidgetToolbarRepository#register()`} method. The following example comes from the {@link module:image/imagetoolbar~ImageToolbar} plugin:

ma2ciek · 2018-09-19T14:59:26Z

I've checked and docs should be fine now.

pjasiun · 2018-09-19T15:53:10Z

src/widgettoolbarrepository.js

+ *				const widgetToolbarRepository = editor.plugins.get( WidgetToolbarRepository );
+ *
+ *				widgetToolbarRepository.register( 'image', {
+ *					toolbarItems: editor.config.get( 'image.toolbar' ),


Maybe just items?

We can change it. I'm ok with both names.

Then change it. It is shorter and have the same meaning in this context.

pjasiun · 2018-09-19T15:56:42Z

src/widgettoolbarrepository.js

+
+		toolbarView.fillFromConfig( toolbarItems, editor.ui.componentFactory );
+
+		if ( this._toolbars.has( toolbarId ) ) {


This check could be moved to the beginning of this method.

ma2ciek force-pushed the t/ckeditor5-ui/442 branch from a6dd351 to c250edf Compare September 18, 2018 16:59

ma2ciek added 11 commits September 18, 2018 19:41

WIP - WidgetToolbar.

6d6f600

Added API docs for the WidgetToolbar.

0b68ce1

Various fixes to widget toolbar.

b16ff1b

Added simple integration tests.

fc82198

Added tests for remove() method.

08b3d46

Added more tests.

fa920a2

Improved tests.

69927c9

Code style improvements.

4a0cfef

Improved tests.

a2bd398

Code style improvements.

b2f47b2

Cleaned up.

39eba17

ma2ciek force-pushed the t/ckeditor5-ui/442 branch from e075c16 to 39eba17 Compare September 18, 2018 17:41

pjasiun reviewed Sep 19, 2018

View reviewed changes

ma2ciek added 4 commits September 19, 2018 13:38

Changed WidgetToolbar to WidgetToolbarRepository.

95ac5f2

Simplified default balloon class name.

261ef60

Changed isVisible to whenVisible.

ab05ffe

Fixed errors.

875bb8b

Reinmar suggested changes Sep 19, 2018

View reviewed changes

pjasiun reviewed Sep 19, 2018

View reviewed changes

Fixed option name.

6d5bf11

Reinmar reviewed Sep 19, 2018

View reviewed changes

tests/widgettoolbarrepository.js Outdated

} );

} );

describe( 'deregister', () => {

Copy link

Member

Reinmar Sep 19, 2018

Choose a reason for hiding this comment

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

deregister()

Reinmar reviewed Sep 19, 2018

View reviewed changes

tests/widgettoolbarrepository.js Outdated

} );

} );

describe( 'isRegistered', () => {

Copy link

Member

Reinmar Sep 19, 2018

Choose a reason for hiding this comment

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

isRegistered()

Removed deregister and isRegistered functions.

e4b1151

ma2ciek changed the title ~~Introduce a factory for widget toolbars~~ Introduce the widget toolbar repository Sep 19, 2018

pjasiun reviewed Sep 19, 2018

View reviewed changes

Improved API docs.

9d46782

Reinmar reviewed Sep 19, 2018

View reviewed changes

API docs fixes.

d7e9b2c

Reinmar reviewed Sep 19, 2018

View reviewed changes

Reinmar suggested changes Sep 19, 2018

View reviewed changes

ma2ciek added 4 commits September 19, 2018 15:54

API docs fixes.

4e9e587

API docs improvements.

8c1bd96

Added missing docs.

4c72221

Fixed failing tests on FF.

e2cc5fc

ma2ciek force-pushed the t/ckeditor5-ui/442 branch from e864de8 to e2cc5fc Compare September 19, 2018 14:52

pjasiun reviewed Sep 19, 2018

View reviewed changes

Fixed description in tests.

3fe23d3

Reinmar approved these changes Sep 19, 2018

View reviewed changes

ma2ciek added 3 commits September 19, 2018 18:27

Changed order in the register function.

1711a70

Renamed toolbarItems to items.

0d72ab3

Added API docs to the _showToolbar() method.

031e370

pjasiun merged commit bc45176 into master Sep 20, 2018

pjasiun deleted the t/ckeditor5-ui/442 branch September 20, 2018 09:34

ma2ciek mentioned this pull request Oct 9, 2019

WidgetToolbarRepository could warn user if it gets empty array of items ckeditor/ckeditor5#4591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the widget toolbar repository #54

Introduce the widget toolbar repository #54

ma2ciek commented Sep 18, 2018 •

edited by pomek

Loading

coveralls commented Sep 18, 2018 •

edited

Loading

pjasiun Sep 19, 2018

pjasiun Sep 19, 2018 •

edited

Loading

Reinmar left a comment

pjasiun Sep 19, 2018 •

edited

Loading

ma2ciek Sep 19, 2018

Reinmar Sep 19, 2018

ma2ciek commented Sep 19, 2018

Reinmar Sep 19, 2018

Reinmar Sep 19, 2018

pjasiun Sep 19, 2018

pjasiun Sep 19, 2018

ma2ciek Sep 19, 2018

ma2ciek Sep 19, 2018

pjasiun Sep 19, 2018

ma2ciek Sep 19, 2018

ma2ciek Sep 20, 2018

pjasiun Sep 19, 2018

ma2ciek Sep 19, 2018

Reinmar Sep 19, 2018

Reinmar Sep 19, 2018

Reinmar Sep 19, 2018

Reinmar left a comment

Reinmar Sep 19, 2018

ma2ciek commented Sep 19, 2018

pjasiun Sep 19, 2018

ma2ciek Sep 19, 2018

pjasiun Sep 20, 2018 •

edited

Loading

ma2ciek Sep 20, 2018

pjasiun Sep 19, 2018 •

edited

Loading


		toolbarView.fillFromConfig( toolbarItems, editor.ui.componentFactory );

		if ( this._toolbars.has( toolbarId ) ) {

Introduce the widget toolbar repository #54

Introduce the widget toolbar repository #54

Conversation

ma2ciek commented Sep 18, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

pjasiun Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Reinmar left a comment

Choose a reason for hiding this comment

pjasiun Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2ciek commented Sep 19, 2018

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

Reinmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2ciek commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

ma2ciek commented Sep 18, 2018 •

edited by pomek

Loading

coveralls commented Sep 18, 2018 •

edited

Loading

pjasiun Sep 19, 2018 •

edited

Loading

pjasiun Sep 19, 2018 •

edited

Loading

pjasiun Sep 20, 2018 •

edited

Loading

pjasiun Sep 19, 2018 •

edited

Loading