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: "user followed you" notification #505

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

Conversation

steven-fox
Copy link
Collaborator

Closes #503

Changes are a little hacky due to the current way we handle rendering/redirecting notifications.

pinkary.followed.notification.mp4

@steven-fox steven-fox changed the title Feat/user followed notification Feat: "user followed you" notification Aug 16, 2024
Copy link
Collaborator

@CamKem CamKem left a comment

Choose a reason for hiding this comment

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

Looks good!

I gotta think though, it would be better for the sake of understanding the code, to extract a notification card of the common elements used & then create 1 blade components per notification & use a match in the notifications/index.blade.php. It would help remove all of those conditionals & provide better readability.

Maybe in a follow up PR...

@steven-fox
Copy link
Collaborator Author

Looks good!

I gotta think though, it would be better for the sake of understanding the code, to extract a notification card of the common elements used & then create 1 blade components per notification & use a match in the notifications/index.blade.php. It would help remove all of those conditionals & provide better readability.

Maybe in a follow up PR...

Couldn’t agree more! #506 😉😁

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.

Followers notifications
2 participants