Reader: connect new combined cards component to streams #11875

samouri · 2017-03-08T02:03:49Z

This PR adds the new ReaderCombinedCard component (#11407) to streams.

It will bundle up consecutive posts from the same feed or site into one combined card rather than many individual post cards.

Have you ever been frustrated about specific RSS feeds overcrowding your Reader streams? Well if so, this here commit is one you will love! It lumps same-site cards together to save you some valuable screen real estate!

matticbot · 2017-03-08T02:03:50Z

Test live: https://calypso.live/?branch=add/reader/connect-combinedcards

bluefuton · 2017-03-08T12:43:47Z

client/reader/stream/index.jsx

+/**
+ * Returns true if two postKeys are for the same siteId or feedId.
+ *
+ * @param {*} postKey1


{*} - should always be an object right?

Updated 👍

bluefuton · 2017-03-08T12:43:58Z

client/reader/stream/index.jsx

+ *
+ * @param {*} postKey1
+ * @param {*} postKey2
+ */


Can we add a @returns too?

bluefuton · 2017-03-08T12:44:46Z

client/reader/stream/index.jsx

+	}
+}
+
+const combineCards = ( postKeys ) => postKeys.reduce(


Can we add a comment explaining what this does?

bluefuton · 2017-03-08T12:45:10Z

client/reader/stream/index.jsx

+const combineCards = ( postKeys ) => postKeys.reduce(
+	( accumulator, postKey ) => {
+		const lastPostKey = last( accumulator );
+		if ( sameSite( lastPostKey, postKey ) && ( ! lastPostKey.postIds || lastPostKey.postIds.length < 5 ) ) {


Can we extract 5 to a const? Not clear what the limit is for.

i think it was just an arbitrary limit to limit the size of a combined block

👍 I've moved it to MAX_COMBINED_CARD_POSTS.

bluefuton · 2017-03-08T14:37:53Z

Looks like it can go a bit pear-shaped when new posts arrive (clicked on the "1 new post" pill):

bluefuton · 2017-03-08T15:35:07Z

We also need to hand the combined card a site and feed (we've done this with ConnectedReaderPostCardAdapter and ReaderPostCardAdapterFluxContainer for regular posts).

blowery · 2017-03-08T15:54:23Z

client/reader/stream/index.jsx

+						postKey={ postKey }
+						index={ index }
+						key={ `combined-card-${ index }` }
+						onClick={ post => showSelectedPost( {


don't want this to be an anonymous function. it'll cause the combo card to re-render every time we do a render pass, even if nothing changed.

blowery · 2017-03-08T19:13:13Z

client/reader/stream/index.jsx

@@ -211,7 +211,10 @@ class ReaderStream extends React.Component {
 		}

 		if ( config.isEnabled( 'reader/combined-cards' ) ) {
-			items = combineCards( items );
+			// Create combined cards unless we're on a site or feed stream
+			if ( ! startsWith( store.id, 'feed:' ) && ! startsWith( store.id, 'site:' ) ) {


maybe pass this in as a prop to the stream?

I thought about that - but what's the best format to do that (capturing the startsWith malarkey too)? A regex?

Ah, I see now. We want a showCombinedCards prop or similar on the stream component itself.

samouri · 2017-03-08T19:40:12Z

client/reader/stream/index.jsx

+const combineCards = ( postKeys ) => postKeys.reduce(
+	( accumulator, postKey ) => {
+		const lastPostKey = last( accumulator );
+		if ( sameSite( lastPostKey, postKey ) && ( ! lastPostKey.postIds || lastPostKey.postIds.length < MAX_COMBINED_CARD_POSTS ) ) {


if we aren't including this on site-stream/feed-stream we can nix the: ( ! lastPostKey.postIds || lastPostKey.postIds.length < MAX_COMBINED_CARD_POSTS )

samouri · 2017-03-08T19:40:47Z

client/reader/stream/index.jsx

@@ -150,6 +209,14 @@ class ReaderStream extends React.Component {
 		if ( ! this.state || posts !== this.state.posts || recs !== this.state.recs ) {
 			items = injectRecommendations( posts, recs );
 		}
+
+		if ( config.isEnabled( 'reader/combined-cards' ) ) {


nice work @bluefuton!

blowery · 2017-03-08T20:31:15Z

client/reader/stream/index.jsx

@@ -438,6 +503,22 @@ class ReaderStream extends React.Component {
 				/>;
 		}

+		if ( postKey.isCombination ) {
+			const handleClick = post => showSelectedPost( {


this has the same problem as before :D you're making a new function for every combined card every time render runs. That means the combined card is getting different props and cannot take advantage of shouldComponentUpdate to avoid re-rendering.

I'm afraid they all have this issue then: https://github.com/Automattic/wp-calypso/pull/11875/files/757a735aed29c4813ea40a346dd5a0f95e29dc93#diff-52f7f0c436e060a64c7081104b31880aR523.

could also be related to the rerender issue #11856

@blowery, lets handle that in a separate PR and keep this focused on the new component

fraying

Lookin good! Posted design tweaks to #11913.

fraying · 2017-03-09T00:40:43Z

Bug Note: Combined Cards currently break keyboard navigation. Active card moves down ("j") until you hit a CC then it just goes away. If you keep hitting "j" the last post before the CC selects and deselects.

samouri · 2017-03-09T15:01:58Z

config/development.json

@@ -121,7 +121,7 @@
 		"press-this": true,
 		"push-notifications": true,
 		"reader": true,
-		"reader/combined-cards": true,
+		"reader/combined-cards": false,


samouri · 2017-03-09T15:02:09Z

client/reader/stream/x-post.jsx

@@ -22,7 +22,7 @@ class CrossPost extends PureComponent {
 		xMetadata: React.PropTypes.object.isRequired,
 		xPostedTo: React.PropTypes.array,
 		handleClick: React.PropTypes.func.isRequired,
-		translate: React.PropTypes.func.isRequried,


samouri added 2 commits March 7, 2017 20:47

Reader: Hook up the new CombinedCard component to streams

f5a456e

Have you ever been frustrated about specific RSS feeds overcrowding your Reader streams? Well if so, this here commit is one you will love! It lumps same-site cards together to save you some valuable screen real estate!

a few fixes to make clicking work

50b7be1

samouri added the [Status] In Progress label Mar 8, 2017

samouri self-assigned this Mar 8, 2017

samouri requested a review from fraying March 8, 2017 02:03

matticbot added the [Size] L Large sized issue label Mar 8, 2017

bluefuton changed the title ~~Reader: connecting combined cards~~ Reader: connect new combined cards component to streams Mar 8, 2017

bluefuton added the [Feature] Reader The reader site on Calypso. label Mar 8, 2017

bluefuton reviewed Mar 8, 2017

View reviewed changes

Hide combined cards behind a feature flag

c8b8ec1

matticbot added the [Size] L Large sized issue label Mar 8, 2017

bluefuton added 2 commits March 8, 2017 14:50

Ditch generic link style for .reader__content a

e2ece5e

Override standard .card styles in stream

60e2941

JSdoc fixes

f0b8bd3

blowery reviewed Mar 8, 2017

View reviewed changes

pick up the sites and feeds in the combo card

56634ce

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Mar 8, 2017

bluefuton added 4 commits March 8, 2017 16:53

Fix feed ID key

617383c

Move max number of combined card posts to a constant

a07c146

Create combined cards unless we're on a site or feed stream

4106a5a

Left-align posts when featured asset is missing

60ecffe

blowery reviewed Mar 8, 2017

View reviewed changes

samouri commented Mar 8, 2017

View reviewed changes

addressed pr feedback

6074f6b

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 8, 2017

turn combined cards off for a8c stream

757a735

samouri added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 8, 2017

samouri mentioned this pull request Mar 8, 2017

Perfect the design of CombinedCards #11913

Closed

blowery reviewed Mar 8, 2017

View reviewed changes

moved onClick handler to the component instead of inside render function

3badd49

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 8, 2017

fraying reviewed Mar 8, 2017

View reviewed changes

bluefuton added 2 commits March 9, 2017 14:57

Enable in wpcalypso

3d3b04d

Fix x-post proptype typo

941206b

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 9, 2017

Fix shouldCombineCards prop typos

e472561

samouri commented Mar 9, 2017

View reviewed changes

bluefuton merged commit ebed741 into master Mar 9, 2017

bluefuton deleted the add/reader/connect-combinedcards branch March 9, 2017 15:07

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: connect new combined cards component to streams #11875

Reader: connect new combined cards component to streams #11875

samouri commented Mar 8, 2017 •

edited by bluefuton

Loading

matticbot commented Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 8, 2017

blowery Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton commented Mar 8, 2017

bluefuton commented Mar 8, 2017

blowery Mar 8, 2017

blowery Mar 8, 2017

bluefuton Mar 8, 2017

bluefuton Mar 9, 2017

samouri Mar 8, 2017 •

edited

Loading

samouri Mar 8, 2017

blowery Mar 8, 2017

samouri Mar 8, 2017 •

edited

Loading

samouri Mar 8, 2017

fraying left a comment

fraying commented Mar 9, 2017

samouri Mar 9, 2017

samouri Mar 9, 2017

Reader: connect new combined cards component to streams #11875

Reader: connect new combined cards component to streams #11875

Conversation

samouri commented Mar 8, 2017 • edited by bluefuton Loading

matticbot commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluefuton commented Mar 8, 2017

bluefuton commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraying left a comment

Choose a reason for hiding this comment

fraying commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri commented Mar 8, 2017 •

edited by bluefuton

Loading

samouri Mar 8, 2017 •

edited

Loading

samouri Mar 8, 2017 •

edited

Loading