Skip to content
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

Pinned posts #2771

Merged
merged 19 commits into from
Sep 26, 2024
Merged

Pinned posts #2771

merged 19 commits into from
Sep 26, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Aug 31, 2024

Behavior on author feed:

  • if pinned post is in the first page of results, filter it from its normal position on that page and set it as the first post along with #reasonPin
  • if pinned post is on subsequent pages, do not filter it, it will appear duplicated
  • viewer.pinned remains the same in either case, and should be true

Behavior in other feeds:

  • viewer.pinned should reflect profile.pinnedPost
Screenshot 2024-08-31 at 23 13 41

@mozzius mozzius force-pushed the samuel/pinned-posts branch from f145759 to 456c8f9 Compare September 23, 2024 16:50
@estrattonbailey
Copy link
Member

Ah one thing: if you pin a reply, it only shows up in your posts & replies 🤔

@mozzius mozzius marked this pull request as ready for review September 25, 2024 15:35
Comment on lines 72 to +82
// @NOTE the feed item types in the protos for author feeds and timelines
// technically have additional fields, not supported by the mock dataplane.
export type FeedItem = { post: ItemRef; repost?: ItemRef }
export type FeedItem = {
post: ItemRef
repost?: ItemRef
/**
* If true, overrides the `reason` with `app.bsky.feed.defs#reasonPin`. Used
* only in author feeds.
*/
authorPinned?: boolean
}
Copy link
Member Author

@mozzius mozzius Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I'm least sure about - is it ok to add this extra field here?

(comment continued here)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this type of a bit funny and could honestly probably find a cozier spot. This type is only used as a parameter for both hydration & views. So you should be good to add this extra property here. The lil note is appreciated 👍

Comment on lines 102 to 109
const pinnedItem = {
post: {
uri: actor.profile.pinnedPost.uri,
cid: actor.profile.pinnedPost.cid,
},
repost: undefined,
authorPinned: true,
}
Copy link
Member Author

@mozzius mozzius Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set authorPinned here, I'm not expecting the value to come from the dataplane or anything so I imagine it should be fine (but would be good to confirm)

@@ -186,6 +194,10 @@
"repost": { "type": "string", "format": "at-uri" }
}
},
"skeletonReasonPin": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like that this could be used in the future for a feed gen to "pin" a post to the top of a feed

@@ -44,7 +44,8 @@
"like": { "type": "string", "format": "at-uri" },
"threadMuted": { "type": "boolean" },
"replyDisabled": { "type": "boolean" },
"embeddingDisabled": { "type": "boolean" }
"embeddingDisabled": { "type": "boolean" },
"pinned": { "type": "boolean" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, does this affect the presentation of a post elsewhere in the app (other than a user's own author feed) when viewing their own pinned post?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use it to show "Pin post" or "Unpin post" in the context menu

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

i'll toss a changeset in here and get it merged 👌

@dholms dholms merged commit 2676206 into main Sep 26, 2024
10 checks passed
@dholms dholms deleted the samuel/pinned-posts branch September 26, 2024 23:26
@github-actions github-actions bot mentioned this pull request Sep 26, 2024
This was referenced Sep 26, 2024
@@ -0,0 +1,17 @@
import { Kysely } from 'kysely'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend isn't using Scylla? What is this kysely about? Just curious

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the Postgres-based reference implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants