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

Editable: Add prop types to warn on non-array value #2344

Merged
merged 2 commits into from
Aug 11, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 10, 2017

Related: #2341

This pull request seeks to enhance Editable to log a warning when passed a non-Array value. As seen in #2341, it can be tempting to pass a plain text string to Editable, but these will not be parseable on subsequent editor sessions.

Testing instructions:

Verify that no warnings are logged for valid Editable usage (e.g. paragraph).

[Assuming #2341 has not yet been merged, ] verify that a warning is logged when rendering saved post with (non-formatted) Cover Image:

  1. Navigate to Gutenberg > New Post
  2. Insert a Cover Image
  3. Assign an image to the block
  4. Add some text to the cover image
  5. Save post
  6. Refresh page

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Aug 10, 2017
@ellatrix
Copy link
Member

I like this idea, but shouldn't we then have it for all props?

@aduth
Copy link
Member Author

aduth commented Aug 10, 2017

I like this idea, but shouldn't we then have it for all props?

I am conflicted, because as of yet I've mostly discouraged propTypes, since Facebook themselves do not use them internally and are already making strides in 16.x to move them externally, perhaps a sign of future unavailability.

With that in mind, in this case I was going to start with a constructor check on the prop, when realizing that propTypes is already so convenient (and also, this component has contextTypes, so it's hard to pretend they don't exist).

In retrospect, the constructor check may not have been all that ugly, if even the same result, and could help reinforce some consistency about how we (don't) use propTypes.

Do you have any thoughts?

@aduth aduth merged commit 9b1c7fd into master Aug 11, 2017
@aduth aduth deleted the add/editable-proptypes branch August 11, 2017 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants