-
Notifications
You must be signed in to change notification settings - Fork 213
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: squad other section #2279
base: main
Are you sure you want to change the base?
Conversation
: post, | ||
); | ||
|
||
const updateMap = { |
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.
The final idea is to have one cache for post states, regardless of who needs it.
As the "intermediate" solution I thought we could have this mapping per query to say what transformer to run on the provided cache. (one issue still is how to update multiple places)
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 suggest looking for normalized cache solutions or ignoring it, and in any way I wouldn't recommend doing it in the same PR. Developing one ourselves is too much effort and bug-prone
</p> | ||
</div> | ||
<div className={classNames(className, 'flex flex-col gap-6')}> | ||
<ActiveFeedContextProvider items={similarPosts} queryKey={queryKey}> |
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.
Items here can actually be removed, but had to leave it for legacy stuff for now.
() => ({ | ||
items, | ||
queryKey, | ||
onClick: onClick || onClickDefault, |
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.
We either can overwrite or keep the default click action.
const { feedRef } = useActiveFeedContext(); | ||
|
||
return ( | ||
<div className="grid grid-cols-2 gap-4" ref={feedRef}> |
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.
If this is to be a generic component, I suggest accepting classes from the outside and set it with default value
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.
Good point indeed, most likely will take default "feed settings" and can get overwritten from outside.
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.
Great work on the POC, appreciate the effort 🚀
Excited to plan and move it forward.
{items.map((item, i) => { | ||
return <LeanFeedItemComponent key={i} item={item} />; | ||
})} |
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.
It could accept children
as a function. So we can call:
Render cards:
<LeanFeed>
{({ items }) => {
return items.map((item, i) => {
return <LeanFeedItemComponent key={i} item={item} />;
})
}}
</LeanFeed>
Add ad to the first place:
<LeanFeed>
{({ items }) => {
return items.map((item, i) => {
if (i === 0) {
return (
<>
<Ad />
<LeanFeedItemComponent key={i} item={item} />;
</>
)
}
return <LeanFeedItemComponent key={i} item={item} />;
})
}}
</LeanFeed>
Of course internal render prop function is in fact just a component:
const FeedWithAds = {({ items }) => {
return items.map((item, i) => {
return <LeanFeedItemComponent key={i} item={item} />;
})
}}
<LeanFeed>
{FeedWithAds}
</LeanFeed>
I think conceptually it would allow us to split view and business logic.
update: (oldPost: Post) => Partial<Post>, | ||
): ((args: { id: string }) => Promise<() => void>) => | ||
async ({ id }) => { | ||
await queryClient.cancelQueries(queryKey); |
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.
Why cancel queries?
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.
Won't be needed, was more a copy-paste from the existing function :)
const { | ||
onPromotePost, | ||
onBanPost, | ||
canDeletePost, | ||
onDeletePost, | ||
canPinPost, | ||
onPinPost, | ||
onBlockSource, | ||
onBlockTag, | ||
onClick: onClickDefault, | ||
onBookmark, | ||
onUpvote, | ||
onDownvote, | ||
onHidePost, | ||
} = useLeanPostActions({ | ||
queryKey, | ||
}); | ||
|
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.
Why do we need this top level, it should be called just in nested components?
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.
This can actually move into the nested components indeed.
The only exposure here I think was in case you can't to overwrite handlers?
Not sure if it's needed for that.
export default function useLeanPostActions({ | ||
queryKey, | ||
transformKey = 'further-reading', | ||
}) { |
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 like the idea but I think component would need to be:
- split up into smaller parts/hooks
- based on your comment here it should support upvoting from single post as well
- we should also see if it can utilize mutation subscriptions to sync state between different caches (as current hooks do)
- should it solve props drilling in cards? feels like it should
- don't get me wrong it looks great just with current functionality I think it's lacking any benefits vs current set of hooks 🙏 🚀
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.
Yeah 100% inline with the idea, Indeed the mutation subscription could potentially just check all caches for that postId and do something with it :)
And fully agree it should be split up again in singular logic, this was easier from a start point only.
[ | ||
items, | ||
onClick, | ||
onBookmark, | ||
onUpvote, | ||
onDownvote, | ||
onHidePost, | ||
onPromotePost, | ||
onBanPost, | ||
canDeletePost, | ||
onDeletePost, | ||
canPinPost, | ||
onPinPost, | ||
onBlockSource, | ||
onBlockTag, | ||
queryKey, | ||
], |
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.
We need to be careful, this currently is going to make feed rendering as slow as it is now or worse.
Basically it should function the same as LeanOptionsButton
where hooks down the render tree call useLeanPostActions
and context only provides bare minimum like items and queryKey.
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.
How would we go about overwrites for onClick
for instance in that case?
These overwrites per feed are pretty much the only scenario for it now.
Would love to hear your thoughts on this.
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.
You do the custom logic in that specialized component?
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.
But let's say it's canPinPost
the page itself defines this.
The specialized component would have no way of knowing about it, unless it's contexted/prop drilled correct? (as it's implementation sits quite deep)
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.
canPinPost
would be something that is in the context then yes and component can decide what to do with it.
}; | ||
|
||
export default function useLeanPostActions({ | ||
queryKey, |
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.
Technically, if we give single post query key it should invalidate single post cache?
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.
Yep!
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/MainLayout.tsx # packages/shared/src/components/cards/ActionButtons.tsx # packages/shared/src/components/modals/common.tsx # packages/shared/src/components/post/PostActions.tsx # packages/shared/src/components/post/SquadPostContent.tsx # packages/shared/src/components/post/SquadPostWidgets.tsx # packages/shared/src/hooks/useOnPostClick.ts # packages/shared/src/hooks/usePostFeedback.ts
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/PostOptionsMenu.tsx # packages/shared/src/components/cards/ActionButtons.tsx # packages/shared/src/components/cards/Card.tsx # packages/shared/src/components/post/SquadPostWidgets.tsx # packages/shared/src/contexts/ActiveFeedContext.tsx
…-other-section # Conflicts: # packages/shared/src/components/squads/SquadFeedHeading.tsx
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/MainFeedLayout.tsx # packages/shared/src/components/MainLayout.tsx # packages/shared/src/components/post/SquadPostContent.tsx # packages/shared/src/components/utilities/common.tsx # packages/shared/src/contexts/ActiveFeedContext.tsx # packages/shared/src/hooks/useFeed.ts # packages/shared/src/hooks/useFeedLayout.ts
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/MainFeedLayout.tsx # packages/shared/src/components/feeds/FeedContainer.tsx # packages/shared/src/hooks/useFeedLayout.ts # packages/webapp/pages/onboarding.tsx
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/MainFeedLayout.tsx # packages/shared/src/components/PostOptionsMenu.tsx # packages/shared/src/components/feeds/FeedContainer.tsx # packages/shared/src/components/modals/common.tsx # packages/shared/src/hooks/useFeedLayout.ts # packages/shared/src/hooks/useOnPostClick.ts # packages/webapp/components/layouts/MainFeedPage.tsx # packages/webapp/pages/onboarding.tsx
…-other-section # Conflicts: # packages/shared/src/components/Feed.tsx # packages/shared/src/components/buttons/Button.tsx # packages/shared/src/components/cards/Card.tsx # packages/shared/src/components/feeds/FeedContainer.tsx # packages/shared/src/components/post/SquadPostContent.tsx # packages/shared/src/components/post/SquadPostWidgets.tsx # packages/shared/src/components/squads/SquadFeedHeading.tsx # packages/shared/src/components/utilities/common.tsx # packages/shared/src/hooks/useFeed.ts # packages/shared/src/hooks/useFeedLayout.ts # packages/shared/src/hooks/useOnPostClick.ts # packages/webapp/components/layouts/MainFeedPage.tsx # packages/webapp/pages/onboarding.tsx # packages/webapp/pages/sources/[source].tsx
Changes
NOTE:
WIP: lots of TODO's and TS errors, but love to hear your overall thoughts.
Idea:
Events
Did you introduce any new tracking events?
Don't forget to update the Analytics Taxonomy sheet
Log the new/changed events below:
Please make sure existing components are not breaking/affected by this PR
Manual Testing
On those affected packages:
Did you test the modified components media queries?
Did you test on actual mobile devices?
WT-{number} #done