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

feat: show pinned posts on individual account page #2779

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

shuuji3
Copy link
Member

@shuuji3 shuuji3 commented Apr 7, 2024

resolves #305

another attempt inspired by #1921 (by @lazzzis)

Screenshot

Screenshot from 2024-04-08 02-06-43 (copy)

Copy link

stackblitz bot commented Apr 7, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Apr 7, 2024

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 5e1a818
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6622620d09db07000832778b

Copy link

netlify bot commented Apr 7, 2024

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 72edfe7
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/676115fb02c96000082dd7d3
😎 Deploy Preview https://deploy-preview-2779--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lazzzis
Copy link
Contributor

lazzzis commented Apr 7, 2024

Thanks for adding this! This looks awesome! A few concerns:

  1. The timeline paginator will prepend "Show {v} new items" and append "Show origin sites" in certain scenarios. Will these two buttons show in pinned timeline as well?
  2. the user can pin a replied comment. Does this screenshot look good to you? link
Screenshot 2024-04-07 at 11 00 59 AM

@shuuji3
Copy link
Member Author

shuuji3 commented Apr 8, 2024

That's good points.

  1. "Show {v} new items" and "Show origin sites"

Probably the first "Show {v} new items" is only shown if a paginator has a bound stream. Both paginators (pinnedPaginator and accountPaginator) on this page are not using stream, so I think we don't have to worry about that case.

But I haven't tested the second case "Show origin sites", I'm going to check if it affects the pinned posts (maybe I need to hide that footer in the pinned timeline).

  1. Does this screenshot look good to you?

This doesn't look good 😅 Let me try adjusting the position.

@shuuji3 shuuji3 force-pushed the shuuji3/feat/prepend-pinned-post branch from 0fa1e7e to ec4bdc6 Compare April 8, 2024 15:28
@shuuji3
Copy link
Member Author

shuuji3 commented Apr 8, 2024

I adjusted the position and font style to be consistent with other badges.

fix-a
thread-a

I also tested the different patterns (remove the margin when there is no thread line) but I think the initial version is consistent and better.

fix-b
thread-b

@shuuji3 shuuji3 marked this pull request as draft April 12, 2024 03:24
@shuuji3 shuuji3 force-pushed the shuuji3/feat/prepend-pinned-post branch from ec4bdc6 to 5e1a818 Compare April 19, 2024 12:22
@shuuji3
Copy link
Member Author

shuuji3 commented Apr 19, 2024

But I haven't tested the second case "Show origin sites", I'm going to check if it affects the pinned posts (maybe I need to hide that footer in the pinned timeline).

I checked this case but found out I already set :end-massage="false" in this line so this message won't be shown.

https://github.com/elk-zone/elk/pull/2779/files#diff-ea5c53d8a780a38a4c56028e9023299518d6e00488d1d03d1b8215f97dc3d36eR39

The "Show origin sites" text is defined in the #done slot in TimelinePaginator:

<template v-if="context === 'account' " #done="{ items }">
but CommonPaginator does not show the slot if engMessage is false:
<slot v-else-if="state === 'done' && endMessage !== false" name="done" :items="items as U[]">

@shuuji3 shuuji3 marked this pull request as ready for review April 19, 2024 12:36
@ayoayco
Copy link
Member

ayoayco commented May 11, 2024

I think we need this too because I don't see the "see pinned posts" we used to have in the menu when viewing a profile if I remember it correctly.👀 Don't know when we lost that.

Copy link

@sloanlance sloanlance left a comment

Choose a reason for hiding this comment

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

:shipit:

Overall, this looks like a good PR. I'm no expert when it comes to Vue, however.

I think the paginator should be renamed, but it's a minor quibble. I'm happy to see other people not only agree that pinned posts need to be shown, but that you have the knowledge to implement it and open a PR. I hope @elk-zone approves this or even makes their own implementation.

@elk-zone: Why not approve and merge this PR? It's been waiting for about 7 months.

function reorderAndFilter(items: mastodon.v1.Status[]) {
return reorderedTimeline(items, 'account')
}
const paginator = useMastoClient().v1.accounts.$select(account.id).statuses.list({ limit: 30, excludeReplies: true })
const pinnedPaginator = useMastoClient().v1.accounts.$select(account.id).statuses.list({ pinned: true })
const accountPaginator = useMastoClient().v1.accounts.$select(account.id).statuses.list({ limit: 30, excludeReplies: true })

Choose a reason for hiding this comment

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

IMHO, I think better names for accountPaginator would be timelinePaginator, postPaginator, or normalPaginator. Because it's not accounts that are being paginated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you so much for the code review. I agree and updated to the postPaginator.

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 72edfe7
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/676115fb2c0c5c00085a3377

@shuuji3
Copy link
Member Author

shuuji3 commented Dec 19, 2024

Sorry for taking too long to update🙏🏻 That's partly because of the limited bandwidth of members who are holding many projects other than Elk.

I think it's fine to self-merge this PR at this moment as it's already well-reviewed, and there's no objection to adding support for this feature.

@shuuji3 shuuji3 added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit dd4076f Dec 19, 2024
15 checks passed
@shuuji3 shuuji3 deleted the shuuji3/feat/prepend-pinned-post branch December 19, 2024 04:21
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.

missing info boxes in profile and pinned posts
4 participants