-
Notifications
You must be signed in to change notification settings - Fork 18
Enhancement: Make ComponentFactory case-insensitive. #325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor fixes requested.
src/componentfactory.js
Outdated
* @param {String} name | ||
* @returns {String} | ||
*/ | ||
function _componentsMapKey( name ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd simply call it getNormalized( name )
. See https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style#visibility-levels.
src/componentfactory.js
Outdated
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private non–class functions are documented with //
in the project. No need to @ignore
it. See: https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style#comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw two patterns for that. I've used the one looked nicer for me ;) But I guess the other pattern is a standard now.
A documentation explaining that component names are case insensitive would be nice too. At least in |
@jodator, make sure the merge commit message you propose is compatible with the convention. Changelog is generated automatically based on this commit schema. |
@oleq Thanks for reminder. I'm so used to |
Suggested merge commit message (convention)
Enhancement: Make
ComponentFactory
case-insensitive. Closes ckeditor/ckeditor5#5422.Defining toolbar components should not fail if someone use different case than defined by pluign.
Additional information
name
is a string before executing.toLowerCase()
just to not havetrying to call .toLowerCase() of undefined
in console.