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

fix pullquote insert error #3605

Merged
merged 1 commit into from Nov 22, 2017

Conversation

Projects
None yet
3 participants
@vladanost
Contributor

vladanost commented Nov 22, 2017

Description

Fixes #3603
Related PR: #2400
Error when inserting Pullquote

How Has This Been Tested?

Manually tested in the browser

Types of changes

Checked the value for existence before use.

@youknowriad youknowriad merged commit 3399b4c into WordPress:master Nov 22, 2017

2 checks passed

codecov/project 36.92% remains the same compared to 5a9bf8a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 22, 2017

Contributor

👍

Contributor

youknowriad commented Nov 22, 2017

👍

@vladanost vladanost deleted the vladanost:fix/pullquote-insert-error branch Nov 22, 2017

@@ -80,7 +80,7 @@ registerBlockType( 'core/pullquote', {
<blockquote key="quote" className={ className }>
<Editable
multiline="p"
value={ toEditableValue( value ) }
value={ value && toEditableValue( value ) }

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Could we make toEditableValue handle the empty case? Thinking in case we introduce another usage of it. Would be a simple change to use Lodash's variant, which handles empty values:

const toEditableValue = value => map( value, ( subValue => subValue.children ) );
@aduth

aduth Nov 27, 2017

Member

Could we make toEditableValue handle the empty case? Thinking in case we introduce another usage of it. Would be a simple change to use Lodash's variant, which handles empty values:

const toEditableValue = value => map( value, ( subValue => subValue.children ) );

This comment has been minimized.

@vladanost

vladanost Nov 27, 2017

Contributor

@aduth Thanks for the suggestion. I'll try that approach. How do I proceed now since it was merged and I deleted the branch? Do I just restore the branch and push again?

@vladanost

vladanost Nov 27, 2017

Contributor

@aduth Thanks for the suggestion. I'll try that approach. How do I proceed now since it was merged and I deleted the branch? Do I just restore the branch and push again?

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

It'd be fine to create a new separate branch with the proposed change.

@aduth

aduth Nov 27, 2017

Member

It'd be fine to create a new separate branch with the proposed change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment