I/8798 disable commands embed html #8899
Conversation
There was a problem hiding this comment.
LGTM!
One small note, make sure you use Markdown in Suggested merge commit message. This saves time when merging the pull request :)
You should also write it keeping in mind that some of those entries (fix, other, feature) will land in the main CHANGELONG.md file. So the main fix in this PR (in table?) should have (closes #N) and others (see #N) to let people reading the changelog know where they came from.
| @@ -304,6 +304,19 @@ export function findOptimalInsertionPosition( selection, model ) { | |||
| return selection.focus; | |||
| } | |||
|
|
|||
| /** | |||
| * Checks if the selection is on object. | |||
There was a problem hiding this comment.
| * Checks if the selection is on object. | |
| * Checks if the selection is on an object. |
|
|
||
| expect( isSelectionOnObject ).to.be.true; | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
I think we're missing two cases here
<paragraph>fo[o</paragraph><image></image><paragraph>ba]r</paragraph><image>[foo]</image>(this will require allowing$textinimage)
| // @returns {Boolean} | ||
| function isMediaSelected( selection ) { | ||
| const element = selection.getSelectedElement(); | ||
| return element && element.name === 'media' || false; |
There was a problem hiding this comment.
| return element && element.name === 'media' || false; | |
| return !!element && element.name === 'media'; |
is more natural and does not raise confusion regarding the order of operators.
| export function checkSelectionOnObject( selection, schema ) { | ||
| const selectedElement = selection.getSelectedElement(); | ||
|
|
||
| return selectedElement && schema.isObject( selectedElement ) || false; |
There was a problem hiding this comment.
| return selectedElement && schema.isObject( selectedElement ) || false; | |
| return !!selectedElement && schema.isObject( selectedElement ); |
is simpler and does not make you think about the order of operators.
|
What about the horizontal line and page break features? They are also marked as an object in the schema and rendered as a widget in the editing data. |
|
They're fine :) |
Suggested merge commit message (convention)
Fix (table): The
insertTablecommand should be disabled if any object is selected. Closes #8798.Fix (media-embed): The
insertMediaEmbedcommand should be disabled if any non-media object is selected. See #8798.Other (widget): The
checkSelectionOnObjectfunction should be exported by the@ckeditor/ckeditor5-widget/src/utils package. See #8798.Tests (html-embed): The manual test should show the insert table and insert media buttons and describe their correct state. See #8798.