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

Make Archives Block display correct empty message #9419

Merged
merged 2 commits into from Sep 4, 2018

Conversation

Projects
None yet
4 participants
@caxco93
Contributor

caxco93 commented Aug 29, 2018

Description

When creating an Archives Block while having no posts published, the
Archives Block is empty and overlaps with the subsequent Blocks.

This addresses that by setting the correct Block content when there
are no Archives to show.

How has this been tested?

Built in tests and manually

Screenshots

Before:
emptyarchives

After:
emptyarchives2

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Fixes #9418

Make Archives Block display correct empty message
When creating an Archives Block while having no posts published, the
Archives Block is empty and overlaps with the subsequent Blocks.

This addresses that by setting the correct Block content when there
are no Archives to show.
@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 3, 2018

Member

@caxco93: Thanks for the PR!

It seems like this could be simplified further: rather than duplicating the sprintf() call, it could be something like:

$block_content = sprintf(
	'<ul class="%1$s">%2$s</ul>',
	esc_attr( $class ),
	$archives ? $archives : __( 'No archives to show.', 'gutenberg' )
);
Member

pento commented Sep 3, 2018

@caxco93: Thanks for the PR!

It seems like this could be simplified further: rather than duplicating the sprintf() call, it could be something like:

$block_content = sprintf(
	'<ul class="%1$s">%2$s</ul>',
	esc_attr( $class ),
	$archives ? $archives : __( 'No archives to show.', 'gutenberg' )
);
@caxco93

This comment has been minimized.

Show comment
Hide comment
@caxco93

caxco93 Sep 4, 2018

Contributor

Yeah it could be simplified with something along those lines, but with that example you gave we would have an ul with only text inside when there are no archives.

This was actually based on a similar implementation for the empty Comments Block, but now that you point this out, I think that we should stick to a simple if ( empty ) ... else for readability.

what do you think @pento ?

Contributor

caxco93 commented Sep 4, 2018

Yeah it could be simplified with something along those lines, but with that example you gave we would have an ul with only text inside when there are no archives.

This was actually based on a similar implementation for the empty Comments Block, but now that you point this out, I think that we should stick to a simple if ( empty ) ... else for readability.

what do you think @pento ?

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 4, 2018

Member

Oh, I totally missed the <ul> / <div> difference. 🙂

This looks good to me, thanks @caxco93!

Member

pento commented Sep 4, 2018

Oh, I totally missed the <ul> / <div> difference. 🙂

This looks good to me, thanks @caxco93!

@pento

pento approved these changes Sep 4, 2018

@pento pento added this to the 3.8 milestone Sep 4, 2018

@pento pento added the [Type] Bug label Sep 4, 2018

@pento pento merged commit 6cb0130 into WordPress:master Sep 4, 2018

2 checks passed

codecov/project 50.36% remains the same compared to cf74b67
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 6, 2018

Member

Just a little note, we probably should have had a design check on this but we can retro do. In future as this is a state lets get insight here.

Member

karmatosed commented Sep 6, 2018

Just a little note, we probably should have had a design check on this but we can retro do. In future as this is a state lets get insight here.

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