Throw an error when user tries to use inappropriate editor over textarea #173
Conversation
|
@pomek I've change solution to not include unnecessary promise. Thx for feedback. |
src/editor/editor.js
Outdated
| * @param {HTMLElement|String} sourceElementOrData The DOM element that will be the source for the created editor | ||
| * or the editor's initial data. | ||
| */ | ||
| static _assertAllowedSourceElement( sourceElementOrData ) { |
There was a problem hiding this comment.
You added this method to the base editor. But there are editor types where textarea is a valid source element. So, in other words, this method is not that generic and should either be made more specific or moved out of here.
The other problem with it is that it does not actually check allowed elements yet. It checks only the textarea case. So not only this method is not defined in the right place, but also it's called incorrectly.
I think the easiest solution here would be to have this duplicated – there's very little code here. The only thing which needs to be DRY-ed is the error message, so we could keep it here and document in the places where we'll be firing it that it's defined here.
There was a problem hiding this comment.
I've move element check directly to create methods of each class. It's going to be similar to checks for initial data:
https://github.com/ckeditor/ckeditor5-editor-balloon/blob/d5c4162fe018c65604a59c5cd0d0b0f5e53c6574/src/ballooneditor.js#L209-L212
src/editor/editor.js
Outdated
| * or the editor's initial data. | ||
| */ | ||
| static _assertAllowedSourceElement( sourceElementOrData ) { | ||
| if ( isElement( sourceElementOrData ) && sourceElementOrData.tagName.toLowerCase() === 'textarea' ) { |
There was a problem hiding this comment.
You're right, it's completely unnecessary here.
|
I've extracted this one: ckeditor/ckeditor5#1806 to review docs later on. |
There was a problem hiding this comment.
@msamsel I found that all that code is practically duplicated over those 3 repositories. I think that it would make sense to create a single assertion method for that in core & import it in proper editors. Something similarly to attachToForm() method from core/src/editor/utils.
Maybe assertNotUsedOverTextareaElement( initialDataOrElement )? But if you have better name please propose something :D Also move the error docs there.
It also makes sense to extract this if we ever add another editor type.
@jodator that was my thought with that, maybe not in proper place, but to keep common logic in editor's code. It was even implemented here:
AFAIR I talk about this case with @Reinmar and There is very little chance that we introduce new editor type any time soon. |
Suggested merge commit message (convention)
Feature: Provide method to prevent of creating editors in textarea. Closes ckeditor/ckeditor5#1591.
Additional information
Related changes in other repositories: