-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Reader: load a new recommendation after an interaction with a card #5652
Reader: load a new recommendation after an interaction with a card #5652
Conversation
c77704c
to
38064d7
Compare
meta: 'site,post', | ||
origin_site_ID: originSiteId, | ||
origin_post_ID: originPostId, | ||
number: limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
number
doesn't currently do anything in the API endpoint FYI.
The LIMIT
in the SQL query is currently hard-coded to 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
number=
is supported now (thanks for adding @TooTallNate!)
16fc269
to
12b1541
Compare
Ready for 👀 @blowery @TooTallNate |
@bluefuton not sure how this happened - a new card got recommended, but it's a card that already exists? (See Momsicle one below): |
Glitches we've discovered so far:
|
@bluefuton Just posting again here for completeness. I added a 3rd item on the to-do above. They should just count as interactions since they're more prominent and people are likely to click on them vs the post title. I'm unable to reproduce the duplicate cards. |
@bluefuton Would it be possible to use the Discover image as a card header even just for seed posts? Then subsequent cards can just use their site's image as header since we can only grab Discover images for seed posts atm? It looks pretty plain right now - out of 10 cards, there's only 2 with header images and the rest are gray. |
@jancavan @bluefuton wasn't there also a plan to fallback to the canonical image on the featured post if present? |
@jancavan re: "make blavatar, site name, and followers clickable" - clicking any of those already counts as a card interaction and should load a new recommendation. Do you mean to open the full post view (if we have a post) or site stream when clicking those? |
The duplicate cards are coming from the endpoint. They have different recommendation IDs, so there's not much we can do to dedupe in Calypso - we need to fix this on the backend. |
@jancavan let's have a chat about card ordering :) I think we'll need the card display order to match the HTML source order. At the moment new recs are appearing in the wrong spot. |
d981e5f
to
abee2bb
Compare
I thought this was what we were already doing, but cleared this up with @bluefuton. @TooTallNate Can we use the site header then fallback will be Discover featured image (for seed posts) then featured post image?
Blavatar, site name, and followers should jump to the site's stream. |
#5564 will expose |
@bluefuton tried different ways to fix the ordering for cards to flow from L to R instead of Top to Bottom, but it's really not possible with plain CSS. Either we:
Let's just go with (2). With the current size of the cards, it's hard to make the connection between the card the you clicked and new cards that show up, especially in smaller viewports. |
|
f82051c
to
d463183
Compare
Thanks @jancavan. New recommendations are now appended to the end of the list 👍 |
Card sorting issue has been addressed in: #5944 👏 |
ace3196
to
cb9c1ec
Compare
Are we using Discover featured image as fallback yet @bluefuton? This post has a Discover image, but it's showing the gray background: |
No, we don't have access to it in the API response at the moment. We'll either have to add the discover post to |
return Object.assign( {}, state, keyBy( action.recommendations, 'ID' ) ); | ||
// Filter out any recommendations we already have | ||
const newRecommendations = filter( action.recommendations, ( incomingRecommendation ) => { | ||
if ( findIndex( state, ( existingRecommendation ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this can just be return ! find( state, { ID: incomingRecommendation.ID } )
if you use lodash/find
?
if you don't like the negative in the boolean you could use lodash/reject
instead. reject( state, { ID: incomingRecommendation.ID } )
|
- store interacted cards in the Redux tree - get child recommendation ID for card where available - only fetch 1 recommendation from API in recordRecommendationInteraction() - don't accept duplicate recommendations - add test for recommendation insertion order
… each card interaction
cb9c1ec
to
4840cb0
Compare
4840cb0
to
f539fd8
Compare
Looks good! ship away. |
If a user interacts with a recommendation card, a related recommendation card should be loaded and after immediately after it (where available).
How does it work, then?
When a user clicks anywhere on the card, we attempt to grab up to 3 extra recommendations. If we don't have one, we ask the API for new ones.
If we find any, we insert the recommendations at the end of the existing cards.
Note: clicking cards won't always trigger the loading of new recommendations, for example if they're sibling cards of an existing recommendation, or if we already have the recommended sites in the currently loaded cards.
Todo