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

Support for read-only mode #56

Merged
merged 8 commits into from
Nov 27, 2018
Merged

Support for read-only mode #56

merged 8 commits into from
Nov 27, 2018

Conversation

pomek
Copy link
Member

@pomek pomek commented Nov 27, 2018

Suggested merge commit message (convention)

Feature: Introduced the [disabled] property which allows switching the editor to read-only mode. Closes #53.

@coveralls
Copy link

coveralls commented Nov 27, 2018

Pull Request Test Coverage Report for Build 123

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 111: 0.0%
Covered Lines: 37
Relevant Lines: 37

💛 - Coveralls

@@ -372,6 +372,64 @@ describe( 'CKEditor Component', () => {
} );
} );
} );

describe.only( '#disabled', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

❤️

@pomek
Copy link
Member Author

pomek commented Nov 27, 2018

Because ckeditor/ckeditor5#1364 is still open, I added the new property in mentioned PR.

src/ckeditor.jsx Outdated
@@ -103,11 +127,13 @@ CKEditor.propTypes = {
onChange: PropTypes.func,
onInit: PropTypes.func,
onFocus: PropTypes.func,
onBlur: PropTypes.func
onBlur: PropTypes.func,
disabled: PropTypes.oneOfType( [ PropTypes.bool, PropTypes.string ] )
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree here. Why we should support multiple types here? Can't we just support only boolean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? You can do disabled="true" (as I did before I discovered disabled={true}) and it still will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

But disabled="true" isn't the preferred way to pass a boolean option to react component as you find out yourself... IMO we should support more than one type if there're a concept and more logic behind the other types (like string and array of strings).

Copy link
Member Author

Choose a reason for hiding this comment

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

Now the component will accept boolean value only.

@@ -51,6 +56,16 @@ <h2>Sample</h2>
} ),
document.getElementById( 'root' )
);

document.getElementById( 'readOnly' ).addEventListener( 'click', function() {
window.editor.isReadOnly = !window.editor.isReadOnly;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the editor isn't available here? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this code to onInit component's callback.

@ma2ciek ma2ciek merged commit 6765006 into master Nov 27, 2018
@ma2ciek ma2ciek deleted the t/53 branch November 27, 2018 12:06
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

3 participants