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

Test caption length when considering to show editable #684

Merged
merged 4 commits into from
May 8, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented May 5, 2017

Fixes #673
Related: #668

This pull request seeks to resolve an issue where caption and citation fields are shown when the block has no focus and no caption content. In doing so, it also removes invalid paragraphs from the demo post-content.js text.

Implementation notes:

The children matcher always returns an array. An array is truthy even when it has no values. Instead, test length of the array to see if any content is to be shown.

Testing instructions:

Verify that only images, quotes, and embeds with caption text show the caption Editable, except when focused (in which case, Editable should always be shown).

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label May 5, 2017
@aduth aduth requested a review from mtias May 5, 2017 18:02
This was referenced May 5, 2017
aduth added 2 commits May 5, 2017 14:29
Can be undefined for new block. We may want to look to provide default,
but for now simply test truthiness.
@@ -120,7 +120,7 @@ registerBlock( 'core/quote', {
onMerge={ mergeWithPrevious }
showAlignments
/>
{ ( citation || !! focus ) && (
{ ( ( citation && citation.length > 0 ) || !! focus ) && (
Copy link
Member Author

Choose a reason for hiding this comment

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

Preempting a concern here: Yes, this is ugly. Rather than extract to a separate variable, I think we should acknowledge its ugliness and try to avoid it altogether by providing default values for children. This should be addressed separately.

@mtias
Copy link
Member

mtias commented May 8, 2017

Ok, let's merge this.

@mtias mtias merged commit e4d749a into master May 8, 2017
@mtias mtias deleted the fix/673-empty-captions branch May 8, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Block: don't display empty caption when not focused
2 participants