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

fix: card overlay not toggling between upvote and bookmark #3831

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Abhi-Bohora
Copy link
Contributor

Changes

fixes: #1510

Describe what this PR does

The problem here was if i click on upvote and click the bookmark afterwards(upvote should be still clicked) the overlay of the upvote still remains in the card and the overlay the bookmark doesn't appear. but the opposite works fine if i click on bookmark first and than on upvote(bookmark should be still clicked) then it first shows the overlay of bookmark and than overlay of the upvote when each of the specific button is pressed.

so I have come up with this solution to fix that issue, I don't know if this is the best approach to solve this issue. and please i need feedback if there can be easier solution or if i am missing something 🙏

Before:

before.mp4

After:

after.mp4

Events

Did you introduce any new tracking events?

If yes please remove the comment HTML comment tags and fill the table below

Don't forget to update the Analytics Taxonomy sheet

Log the new/changed events below:

Type event_name value
Change/New event name extra: { ... }

-->

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Copy link

vercel bot commented Nov 18, 2024

@Abhi-Bohora is attempting to deploy a commit to the Daily Dev Team on Vercel.

A member of the Team first needs to authorize it.

@Abhi-Bohora
Copy link
Contributor Author

Abhi-Bohora commented Nov 18, 2024

things i have noticed while solving this issue is in the original code if you click on upvote and than click on bookmarks afterwards than this if condition

if (shouldShowOverlay && onShare) {
  return (
    <CardCoverShare
      post={post}
      onCopy={onInteract}
      onShare={() => {
        onInteract();
        onShare(post);
      }}
    />
  );
}

gets triggered both the time. while going otherway if you first click on bookmark and than upvote than each individual if condition gets triggered properly and works fine.

@Abhi-Bohora
Copy link
Contributor Author

Hello @sshanzel thanks for feedback, i have implemented this way tracking the current interaction and showing the overlay accordingly. please have a look and if you have something in mind please suggest me i will try to implement that way. currently this is what i came up with.

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Amazing effort. One last thing then I think we can start testing and see if we can merge. Again, wanted to thank you for all your contribution 🙏

packages/shared/src/hooks/post/usePostShareLoop.ts Outdated Show resolved Hide resolved
@@ -26,11 +26,12 @@ export const useCardCover = ({
onShare,
className = {},
}: UseCardCoverProps): UseCardCover => {
const { shouldShowOverlay, onInteract } = usePostShareLoop(post);
const { shouldShowOverlay, onInteract, currentInteraction } =
usePostShareLoop(post);
const shouldShowReminder = useBookmarkReminderCover(post);
Copy link
Member

Choose a reason for hiding this comment

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

We can now remove this line and have it coming from the hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry but which line are you referring this line ? const shouldShowReminder = useBookmarkReminderCover(post);

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, this exact line 31. Then from the hook, we return shouldShowReminder from inside of it and be used here.

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.

🐛 BUG: bookmarking an upvoted post prioritizes showing the copy link overlay
2 participants