Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Internal: Used beforeChange instead of change to force command read-only mode. #128

Merged
merged 2 commits into from May 30, 2018

Conversation

oskarwrobel
Copy link
Contributor

Suggested merge commit message (convention)

Internal: Used beforeChange instead of change to force Command read-only mode. Closes https://github.com/ckeditor/ckeditor5-utils/issues/171.


Requires

@coveralls
Copy link

coveralls commented May 28, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling db2d2af on t/ckeditor5-util/171 into 6f55c20 on master.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

The change requires tests ;-)

@oskarwrobel
Copy link
Contributor Author

This functionality is tested

it( 'is always falsy when the editor is in read-only mode', () => {
editor.isReadOnly = false;
command.isEnabled = true;
editor.isReadOnly = true;
// Is false.
expect( command.isEnabled ).to.false;
command.refresh();
// Still false.
expect( command.isEnabled ).to.false;
editor.isReadOnly = false;
// And is back to true.
expect( command.isEnabled ).to.true;
} );
it( 'is observable when is overridden', () => {
editor.isReadOnly = false;
command.isEnabled = true;
editor.bind( 'something' ).to( command, 'isEnabled' );
expect( editor.something ).to.true;
editor.isReadOnly = true;
expect( editor.something ).to.false;
} );
.

@Reinmar
Copy link
Member

Reinmar commented May 29, 2018

So why is this change needed? If everything is fine here, then there's no need for a PR. But we know that something is wrong. So let's test it.

@oskarwrobel
Copy link
Contributor Author

Ok, test is added.

@Reinmar Reinmar merged commit 0106d5d into master May 30, 2018
@Reinmar Reinmar deleted the t/ckeditor5-util/171 branch May 30, 2018 07:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants