Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/291: ToolbarView#fillFromConfig should warn when the factory does not provide a component #292

Merged
merged 7 commits into from
Sep 12, 2017

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 23, 2017

Suggested merge commit message (convention)

Other: ToolbarView#fillFromConfig should warn when the factory does not provide a component. Closes ckeditor/ckeditor5#5398. Closes ckeditor/ckeditor5#526.


Additional information

@oleq oleq requested a review from Reinmar August 23, 2017 13:24
* @param {module:ui/componentfactory~ComponentFactory} factory The factory that is missing the component.
*/
log.warn(
'toolbarview-missing-component: There is no such component in the factory.',
Copy link
Member

Choose a reason for hiding this comment

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

This was supposed to be a more meaningful error, but it's still all about esoteric components and factories. I think you should read https://medium.com/@thomasfuchs/how-to-write-an-error-message-883718173322 ;)

this.items.add( factory.create( name ) );
} else {
/**
* There was a problem with expanding the toolbar configuration into toolbar items.
Copy link
Member

Choose a reason for hiding this comment

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

Same here as below – "expanding" – really? There should be a registry of forbidden words. Here, try not to use "expanding" and "factory" and "components" :P

@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2017

R- because of the wording (which is the essential part o this ticket).

@oleq
Copy link
Member Author

oleq commented Sep 8, 2017

Hopefully, this will look more understandable now.

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2017

Since there's only one factory of UI items in the editor I'll remove it from the error details. This will make reading the error easier since there will be only the critical information.

@oleq
Copy link
Member Author

oleq commented Sep 12, 2017

Since there's only one factory of UI items in the editor I'll remove it from the error details. This will make reading the error easier since there will be only the critical information.

Don't forget that ckeditor5-ui is not only about CKEditor, it's a UI framework and it could be used anywhere, each time with a different factory.

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2017

Don't forget that ckeditor5-ui is not only about CKEditor, it's a UI framework and it could be used anywhere, each time with a different factory.

Sure. And this will happen how many times? ;)

Besides, in such special use cases, the developer will be more aware of what they are doing anyway.

@Reinmar Reinmar merged commit 2e63e70 into master Sep 12, 2017
@Reinmar Reinmar deleted the t/291 branch September 12, 2017 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants