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

Inserter: Show messaging when no blocks found #4040

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented Dec 15, 2017

This pull request seeks to add messaging when searching using the inserter reveals no available block options.

Before After
Before After

Testing instructions:

Verify that if no blocks are found by searching the inserter menu that a "No blocks found" message is shown instead.

@jorgefilipecosta

The code looks good to me and things are working as expected.

@@ -255,6 +256,14 @@ export class InserterMenu extends Component {
}
renderCategories( visibleBlocksByCategory ) {
if ( isEmpty( visibleBlocksByCategory ) ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 15, 2017

Member

If the only usage of this logic is when searching it may make sense to make this verification in line 358 where we call render categories for search as it makes things more explicit.

@jorgefilipecosta

jorgefilipecosta Dec 15, 2017

Member

If the only usage of this logic is when searching it may make sense to make this verification in line 358 where we call render categories for search as it makes things more explicit.

This comment has been minimized.

@aduth

aduth Dec 18, 2017

Member

Yeah, I think that makes sense. It's a bit complicated with trying to render the result of this.getVisibleBlocksByCategory inline, since we'd need to both test its non-emptiness and pass as the argument to renderCategories (or call twice, which is wasteful).

Lodash's _.cond could handle this pretty nicely, though maybe a bit less obvious to read?

cond( [
	[ isEmpty, () => (
		<span className="editor-inserter__no-results">
			{ __( 'No blocks found' ) }
		</span>
	) ],
	[ () => true, this.renderCategories ]
] )( this.getVisibleBlocksByCategory( this.getBlockTypes() ) );
@aduth

aduth Dec 18, 2017

Member

Yeah, I think that makes sense. It's a bit complicated with trying to render the result of this.getVisibleBlocksByCategory inline, since we'd need to both test its non-emptiness and pass as the argument to renderCategories (or call twice, which is wasteful).

Lodash's _.cond could handle this pretty nicely, though maybe a bit less obvious to read?

cond( [
	[ isEmpty, () => (
		<span className="editor-inserter__no-results">
			{ __( 'No blocks found' ) }
		</span>
	) ],
	[ () => true, this.renderCategories ]
] )( this.getVisibleBlocksByCategory( this.getBlockTypes() ) );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 19, 2017

Member

This version with lodash cond is nice, but it is not obvious to read as you pointed out. The other possibility would save getVisibleBlocksByCategory in a const before the render but this way we may be computing it without need. I did not think about the complication because of being inline. It looks like our options have their downsides are not better than the current version we have, so I think we can ignore this detail and merge as it is right now.

@jorgefilipecosta

jorgefilipecosta Dec 19, 2017

Member

This version with lodash cond is nice, but it is not obvious to read as you pointed out. The other possibility would save getVisibleBlocksByCategory in a const before the render but this way we may be computing it without need. I did not think about the complication because of being inline. It looks like our options have their downsides are not better than the current version we have, so I think we can ignore this detail and merge as it is right now.

@aduth aduth merged commit 266247d into master Jan 2, 2018

3 checks passed

codecov/project 38.5% (+<.01%) compared to 5d81a0a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the update/inserter-no-blocks-found branch Jan 2, 2018

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