Add 'no posts' message to Latest Posts by Soean · Pull Request #3180 · WordPress/gutenberg

Soean · 2017-10-26T14:20:03Z

Description

Now the placeholder shows a message, if no posts were found. This PR adds a loading state to the block.
resolve #3135

Screenshots:

Loading:

No posts:

codecov · 2017-10-26T14:25:18Z

Codecov Report

Merging #3180 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
- Coverage   31.17%   31.17%   -0.01%     
==========================================
  Files         235      235              
  Lines        6534     6535       +1     
  Branches     1163     1165       +2     
==========================================
  Hits         2037     2037              
+ Misses       3773     3772       -1     
- Partials      724      726       +2

Impacted Files	Coverage Δ
blocks/library/latest-posts/index.js	`9.8% <0%> (-0.2%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ea79aa...3a7657e. Read the comment docs.

jorgefilipecosta · 2017-10-26T17:08:37Z


 			this.state = {
 				latestPosts: [],
+				isLoading: true,


Hi, @Soean thank you for your contribution!
Would it be possible to avoid the need for an isLoading flag by setting the initial state of latestPosts to null?
This way we would be able to differentiate between the state lastest posts are not yet available and latestPosts are empty. By avoiding loading flags and just checking data availability, if later we decide to implement an offline mechanism it may make things simpler.

Hey @jorgefilipecosta, thanks for your review. I added the changes to the PR.

mtias · 2017-10-27T14:21:08Z

 						label={ __( 'Latest Posts' ) }
 					>
-						<Spinner />
+						{ placeholderContent }


This could be inlined here:

{ condition ? <Spinner /> : __( 'No posts found.' ) }

Thanks for your feedback, I inlined the condition.

jorgefilipecosta · 2017-10-27T14:41:33Z


-			if ( ! latestPosts.length ) {
+			const hasPosts = Array.isArray( latestPosts ) && latestPosts.length;
+			if ( ! hasPosts ) {


Hi @Soean, if there are no posts we just render the placeholder. So if the user has no posts the user will not be able to change to block settings e.g: number of posts to display. I think if the user does not have posts he should be able to use the inspector to configure the block like if posts existed.

@jorgefilipecosta You are right. Now the inspector controls are also shown on the placeholder view. The BlockControls with align and layoutControls are only visible if posts existed.

Hi @Soean this tests well for me! Nice work :) @mcsf, @gziolo could you have a second look
👀 before merging?

mcsf

This looks good, thanks for the contribution. My recommendations are to address the feedback left here, and then that you rebase your branch on top of the latest origin/master and squash it into one nice commit, so that we can merge it cleanly.

mcsf · 2017-11-02T14:56:04Z

 			const { latestPosts } = this.state;
-			const { setAttributes } = this.props;
+			const { focus, setAttributes } = this.props;
+			const { displayPostDate, align, layout, columns } = this.props.attributes;


Minor, but attributes could be extracted in the line above, so that this line becomes const { … } = attributes.

We can also extract postsToShow here, since it's used several times.

mcsf · 2017-11-02T15:00:36Z

-			if ( ! latestPosts.length ) {
-				return (
-					<Placeholder
+			const inspectorControls = (


Also minor: if this becomes const inspectorControls = focus && ( then we can reduce one whole indentation level for that assignment.

mcsf · 2017-11-02T15:04:23Z

+						{ ! Array.isArray( latestPosts ) ?
+							<Spinner /> :
+							__( 'No posts found.' )
+						}


When using ternaries (or if/else structs, for that matter) we tend to prefer conditions that aren't negated. Also, specifically for ternaries (but not e.g. &&) we prefer to place the operators at the beginning of the breaking lines. So in this case:

Array.isArray( latestPosts ) ? __( 'No posts found.' ) : <Spinner />

I also favor these, but I wonder if a recent ESlint update forced the style applied here, in which case we should update the ESlint config.

Oh, you're right, this has changed. @Soean, you can dismiss my comment on prepending the lines with the operators.

Once we update our ESLint config we can circle back, no worries.

rebase branch

Soean · 2017-11-02T16:46:48Z

@mcsf Thanks for your review. The minor code improvements make sense. I rebased the PR and added the improvements into the last commit.

codebykat · 2017-11-08T07:10:47Z

Tested this and it's working well 👍

gziolo

All comments were addressed. I think it looks good to go. Thanks for your contribution :) Taking into account that it was tested by @codebykat and me. I'm giving 👍

Add loading state

fc935e4

Soean changed the title ~~Add loading state to Latest Posts~~ Add 'no posts' message to Latest Posts Oct 26, 2017

jorgefilipecosta reviewed Oct 26, 2017

View reviewed changes

Init latestPosts with null

88ab229

jorgefilipecosta mentioned this pull request Oct 27, 2017

Implemented query panel in latest posts block. #3198

Merged

mtias reviewed Oct 27, 2017

View reviewed changes

mtias added [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement. labels Oct 27, 2017

Inline spinner condition

f64dfef

jorgefilipecosta reviewed Oct 27, 2017

View reviewed changes

Soean added 2 commits October 28, 2017 19:30

Merge remote-tracking branch 'origin/master' into patch-5

7f87f82

Show inspectorControls also for placeholder.

d20948a

mcsf reviewed Nov 2, 2017

View reviewed changes

Soean added 2 commits November 2, 2017 17:15

Merge remote-tracking branch 'upstream/master' into patch-5

73d44cd

rebase branch

Minor code improvements.

3a7657e

gziolo approved these changes Nov 8, 2017

View reviewed changes

gziolo merged commit eab4b64 into WordPress:master Nov 8, 2017

Conversation

Soean commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots:

Uh oh!

codecov Bot commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soean Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soean commented Nov 2, 2017

Uh oh!

codebykat commented Nov 8, 2017

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Soean commented Oct 26, 2017 •

edited

Loading

codecov Bot commented Oct 26, 2017 •

edited

Loading

Soean Oct 28, 2017 •

edited

Loading