Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ma2ciek opened this issue Sep 20, 2018 · 7 comments
Closed
Labels
package:widget resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 20, 2018

Follow-up of ckeditor/ckeditor5-widget#54.

For now, it might happen that the user will make a typo in the config and without any error, the toolbar will be visible as empty.

This warning could be inside the WidgetToolbarRepository#register() or inside every toolbar (the second approach would be slightly better as it could have the more meaningful message, e.g. The "image.toolbar" config is undefined or an empty string. Please, make sure that your config doesn't contain a typo.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Sep 24, 2018

Recently I've made such a mistake. I've created the following config:

{
	media: {
		toolbar: [ 'comment' ],
	}
}

and I was surprised that the comment button doesn't show up. Finally, I've found that I have a typo and it should be media -> mediaEmbed.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Sep 26, 2018

And I remember that there was some idea in the past to consume the editor configuration (or maybe there wasn't?). That would be super useful to have an information like:

The `config.media` wasn't consumed by any plugin. Probably you miss some plugin or you have a typo in the editor's configuration.

But that would require at least some small changes in every plugin that uses editor's config.

WDYT @Reinmar @pjasiun?

@pjasiun
Copy link

pjasiun commented Sep 26, 2018

The idea is very nice (I do not remember it was suggested before), but as you mentioned we should change plugins to do it...

Or not. We can check if config.get was called on all configuration options and log configuration that was not get.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Sep 26, 2018

Right, we can check config.get calls. But probably it would solve the problem only for one level deep since many plugins get their configuration namespace.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Sep 26, 2018

I'll extract the idea to the ticket in the CKE5 since I think it could be quite nice and low costing.

@pjasiun
Copy link

pjasiun commented Sep 26, 2018

👍 But I think we should handle it in the next iteration when we will introduce debug mode.

@Reinmar
Copy link
Member

Reinmar commented Sep 28, 2018

I'm closing this because widget toolbar should not warn an error. Instead, it should make sure to not show the toolbar at all if it's empty. Why? Because a developer who's integrating CKEditor 5 with his app may want to hide some toolbar and then he/she should be able to configure it to null or whatever falsy value.

Passing a non-predefine (registered) config property to Editor.create() is a different story and #1266 covers it perfectly.

@Reinmar Reinmar closed this as completed Sep 28, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-widget Oct 9, 2019
@mlewand mlewand added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:widget labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants