-
Notifications
You must be signed in to change notification settings - Fork 10
Changed: Use EnterCommand's afterExecute event to alter it's behavior. #29
Conversation
* @param {String} [formatId] The identifier of the heading format that should be applied. It should be one of the | ||
* {@link headings.HeadingsFormat heading formats} provided to the command constructor. If this parameter is not provided, | ||
* the value from {@link headings.HeadingsCommand#defaultFormat defaultFormat} will be used. | ||
* @returns {Object} data Data object, available in {@link core.Command#event:afterExecute} | ||
* @returns {engine.model.Batch} data.batch Batch created and used by the command. |
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.
Leftover?
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.
Yes, I fixed it.
@@ -53,6 +53,15 @@ describe( 'HeadingsCommand', () => { | |||
expect( command.value ).to.equal( format ); | |||
} ); | |||
} | |||
|
|||
it( 'should be equal to defaultFormat if format has not been found', () => { |
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.
Why can't it be null? Why is this change needed now?
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.
This is because when you focus selection inside a model element that is not in formats
array, you get this error:
Uncaught TypeError: Cannot read property 'label' of undefined
.
This is because no option has been found to be displayed in the dropdown.
However, as I think of it now, it should probably be done like it is in CKE4. There is "Format" option there whenever you are inside something that is not recognized by Headings
feature. Right now, in CKE5, you will have "Paragraph" option displayed. Because of that you are unable to actually change to the paragraph using Headings
dropdown.
I think this is worthy of seperate issue because then we have to ask ourselves what is the defaultValue
-- still paragraph
or maybe "format"?
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.
OK, so I'm fine with defaulting to the default format in such a case. But next time please do not squeeze such changes without mentioning them ;)
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 am sorry, this is just the way I are.
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.
One unclear change plus some leftovers.
Fixes ckeditor/ckeditor5#2414