Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

T/17027 Commands event data should be initialized as an empty object #368

Merged
merged 6 commits into from May 12, 2017
Merged

T/17027 Commands event data should be initialized as an empty object #368

merged 6 commits into from May 12, 2017

Conversation

Comandeer
Copy link
Member

Currently if command is not passed an object as a second argument, it fall backs to undefined, causing the unability to pass anything from command to afterCommandExec event.

This change is intended to introduce empty object as a default value for command's event data.

@mlewand mlewand self-requested a review May 12, 2017 12:54
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Idea is good, there are just few minor things to adjust.


this.editor.once( 'afterCommandExec', function( evt ) {
assert.areSame( beforeExecData, evt.data.commandData, 'The same object is passed to afterCommandExec.' );
}, null, null, 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to set the priority.

}, null, null, 1 );

this.editor.addCommand( 'dataFlow', {
exec: function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're testing beforeExecData in beforeCommandExec and afterCommandExec it would make sense to test it in exec too for the completeness.

this.editor.once( 'beforeCommandExec', function( evt ) {
beforeExecData = evt.data.commandData;

assert.isObject( beforeExecData, 'Event data is initialized as an empty object.' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put dots at the end of assertion msg.

@mlewand
Copy link
Contributor

mlewand commented May 12, 2017

I added these minor changes while reviewing, so R+.

@mlewand mlewand merged commit abb2c09 into ckeditor:major May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants