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

Feature/visibility #100

Merged
merged 8 commits into from
Jan 20, 2021
Merged

Feature/visibility #100

merged 8 commits into from
Jan 20, 2021

Conversation

ndrsllwngr
Copy link
Owner

@ndrsllwngr ndrsllwngr commented Jan 20, 2021

  • Move db logs in useSingleMeme to finally promise
  • Fix linter warning
  • Add UnauthorizedPage /403
  • Add auth verification on /meme/[id] for private memes
  • Clear singlememeContext properly while routing to another page

@vercel
Copy link

vercel bot commented Jan 20, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ndrsllwngr/omm/nc0ku6816
✅ Preview: https://omm-git-feature-visibility.ndrsllwngr.vercel.app

@ndrsllwngr
Copy link
Owner Author

ndrsllwngr commented Jan 20, 2021

})
}
}
}, [router, currentMeme, updateCurrent])
Copy link
Owner Author

Choose a reason for hiding this comment

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

Added this useEffect which is similar to the one we have in the autoplayContext. Otherwise we would get a short flash of the old currentMeme when visiting from another page. This useEffect clears the context properly if the user leaves /meme/[id].

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfekt!

@ndrsllwngr ndrsllwngr mentioned this pull request Jan 20, 2021
15 tasks
Comment on lines 22 to 35
{meme.visibility === VISIBILITY.PUBLIC && (
<>
{prevMeme && prevMeme.id ? (
<SlideshowButton name="prev" changeSlide={prevMeme.id} />
) : (
<SlideshowButton name="prev" disabled={true} />
)}
{nextMeme && nextMeme.id && meme.visibility === VISIBILITY.PUBLIC ? (
<SlideshowButton name="next" changeSlide={nextMeme.id} />
) : (
<SlideshowButton name="next" disabled={true} />
)}
</>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really sure if this matches all cases

Copy link
Collaborator

@maxrawlinger maxrawlinger Jan 20, 2021

Choose a reason for hiding this comment

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

I understand the outer condition but not the inner one with "meme.visibility === VISIBILITY.PUBLIC"

Copy link
Owner Author

@ndrsllwngr ndrsllwngr Jan 20, 2021

Choose a reason for hiding this comment

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

Good catch @maxrawlinger the inner is legacy code from an earlier attempt. I am going to remove it!

})
}
}
}, [router, currentMeme, updateCurrent])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfekt!

Copy link
Collaborator

@maxrawlinger maxrawlinger left a comment

Choose a reason for hiding this comment

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

Solid work! 🍕

@maxrawlinger maxrawlinger merged commit e8743d1 into main Jan 20, 2021
@maxrawlinger maxrawlinger deleted the feature/visibility branch January 20, 2021 21:11
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.

2 participants