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

Add support for readOnly prop #206

Merged
merged 7 commits into from
Aug 7, 2019
Merged

Add support for readOnly prop #206

merged 7 commits into from
Aug 7, 2019

Conversation

callmeberzerker
Copy link
Contributor

@callmeberzerker callmeberzerker commented Aug 3, 2019

Fixes [#201].

Copy link
Collaborator

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you @SpearThruster, that was fast 😄 and is looking reeeally close. I’d like the implementation to be slightly different, basically not relying on local state to switch the editor to "readOnly" mode if the prop is used. I think this will make the behavior of this prop more predictable.

Thank you for adding tests as well 🙂I think I’ll also want to add an example of this in Storybook somewhere, but I’m happy to do this myself when merging the changes.

lib/components/DraftailEditor.js Outdated Show resolved Hide resolved
lib/components/DraftailEditor.js Outdated Show resolved Hide resolved
lib/components/DraftailEditor.js Show resolved Hide resolved
@thibaudcolas thibaudcolas added the enhancement New feature or request label Aug 4, 2019
@thibaudcolas thibaudcolas added this to the v1.3.0 milestone Aug 4, 2019
@callmeberzerker
Copy link
Contributor Author

Hi @thibaudcolas

Thanks for the feedback... I updated the code according to your remarks... Will do some npm linking and testing during the next days.

When you are OK with the implementation I will also rebase against master. 👍

Thanks!

Copy link
Collaborator

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you @SpearThruster, this is looking top notch! I spotted a few minor things but I’ll address them while merging.

lib/components/DraftailEditor.js Show resolved Hide resolved
lib/components/DraftailEditor.test.js Outdated Show resolved Hide resolved
@thibaudcolas thibaudcolas merged commit 0995151 into springload:master Aug 7, 2019
@thibaudcolas
Copy link
Collaborator

Merged! 🙂 I’ll be releasing this as part of v1.3.0. I’ll try to get a few more improvements in and release it towards the end of this week.

@callmeberzerker
Copy link
Contributor Author

Awesome, thanks a lot! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants