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: add a notifications page #1478

Merged

Conversation

takanome-dev
Copy link
Contributor

@takanome-dev takanome-dev commented Aug 3, 2023

Description

This pr adds a notifications page to show all the user notifications paginated.

  • add a new notifications page
  • display notifications with paginations
  • filter notifications by type
  • remove the notification dropdown and link to this page
  • should only be accessible by the current logged in user

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes #1444

Mobile & Desktop Screenshots/Recordings

notifications-loading
notifications-empty
notifications

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 📓 docs.opensauced.pizza
  • 🍕 dev.to/opensauced
  • 📕 storybook
  • 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Aug 3, 2023

Deploy Preview for oss-insights ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e61d9f6
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/64d11800c1f9790007d89983
😎 Deploy Preview https://deploy-preview-1478--oss-insights.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.

@netlify
Copy link

netlify bot commented Aug 3, 2023

Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit e61d9f6
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/64d118004cc4d70008f8f8d5
😎 Deploy Preview https://deploy-preview-1478--design-insights.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.

@takanome-dev
Copy link
Contributor Author

cc @bdougie @OgDev-01 @brandonroberts feedback is welcome 🙏

@bdougie
Copy link
Member

bdougie commented Aug 3, 2023

UI is great. I think this leans into our design system well. I let others review the code more closely, but QA wise it works. Navigating here, I think we should have a "view all notifications" link and limit how many we show.

notifications dropdown

We can remove the dropdown and take the user to this page instead. I don't think there is value for having the dropdown anymore with this new page.

Screen Shot 2023-08-03 at 1 46 34 PM

@takanome-dev
Copy link
Contributor Author

UI is great. I think this leans into our design system well. I let others review the code more closely, but QA wise it works. Navigating here, I think we should have a "view all notifications" link and limit how many we show.

I hardcoded the limit to be 10 and there is pagination.

Q. Where should I put the view all notifications link?

@bdougie
Copy link
Member

bdougie commented Aug 3, 2023

Q. Where should I put the view all notifications link?

When you click the notification bell, that is the link. No more dropdown.

@brandonroberts
Copy link
Contributor

@takanome-dev nice work! These should be listed by most recent first. May need an API update on that also

@takanome-dev
Copy link
Contributor Author

These should be listed by most recent first. May need an API update on that also

That's right, how about sorting them before rendering for now?

@takanome-dev takanome-dev marked this pull request as ready for review August 4, 2023 18:32
@takanome-dev takanome-dev added the needs review PRs that need review from core engineering team label Aug 4, 2023
@brandonroberts
Copy link
Contributor

These should be listed by most recent first. May need an API update on that also

That's right, how about sorting them before rendering for now?

That works for now

@brandonroberts
Copy link
Contributor

brandonroberts commented Aug 4, 2023

@takanome-dev I propose changing the path to /user/notifications because its an authenticated route. You can't visit notifications for other users. It will likely simplify some of the middleware logic added also

@brandonroberts brandonroberts added requested changes and removed needs review PRs that need review from core engineering team labels Aug 4, 2023
Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

Made some grammar fixes. For now this works as is, there will be adjustments in the design and UI in the future.

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

Need a test for the new lib function.

lib/utils/get-notification-url.ts Outdated Show resolved Hide resolved
@takanome-dev
Copy link
Contributor Author

but It didn't look like you implemented it yet, so I thought the Layout could be the ProfileLayout hence my suggestions!?

Yo I totally forgot about the footer, thanks for noticing 👍

@babblebey
Copy link
Contributor

Yo I totally forgot about the footer, thanks for noticing 👍

🍕

CHANGELOG.md Outdated Show resolved Hide resolved
middleware.ts Outdated Show resolved Hide resolved
@brandonroberts
Copy link
Contributor

@takanome-dev the changelog updates are still present

@brandonroberts brandonroberts added requested changes and removed needs review PRs that need review from core engineering team labels Aug 7, 2023
@takanome-dev
Copy link
Contributor Author

@takanome-dev the changelog updates are still present

@brandonroberts I need help here 😅 git checkout -- CHANGELOG.md didn't revert it. I also tried git checkout upstream/beta -- CHANGELOG.md but I see new changes. What I'm missing?

@brandonroberts
Copy link
Contributor

@takanome-dev try using the current file contents from beta, using git add . and git commit --no-verify to skip the hooks so it doesn't get formatted again

@takanome-dev
Copy link
Contributor Author

@takanome-dev try using the current file contents from beta, using git add . and git commit --no-verify to skip the hooks so it doesn't get formatted again

It worked, thanks 👍

@brandonroberts
Copy link
Contributor

@takanome-dev Thanks!

@brandonroberts brandonroberts merged commit 022dc69 into open-sauced:beta Aug 8, 2023
open-sauced bot pushed a commit that referenced this pull request Aug 8, 2023
## [1.59.0-beta.7](v1.59.0-beta.6...v1.59.0-beta.7) (2023-08-08)

### 🐛 Bug Fixes

* on page reload Insights page redirecting to Dashboard ([#1517](#1517)) ([397c36e](397c36e))

### 🍕 Features

* add a user notifications page ([#1478](#1478)) ([022dc69](022dc69))
open-sauced bot pushed a commit that referenced this pull request Aug 8, 2023
## [1.59.0](v1.58.0...v1.59.0) (2023-08-08)

### 🧑‍💻 Code Refactoring

* replace supabase/ui in design system typography components ([#1438](#1438)) ([38cfb30](38cfb30))

### 🍕 Features

* add a user notifications page ([#1478](#1478)) ([022dc69](022dc69))
* add conditional routing to single highlight dialog close action ([#1473](#1473)) ([1341cba](1341cba))
* add github link to profile ([#1459](#1459)) ([d42bc6d](d42bc6d))
* add support for highlight.new ([#1487](#1487)) ([3daa5c0](3daa5c0))
* improved the UX on the feeds page and scroll behaviour ([#1506](#1506)) ([31c1593](31c1593))
* onboarding auto fetch timezone ([#1488](#1488)) ([ae5cdd7](ae5cdd7))

### 🐛 Bug Fixes

* Feed page responsiveness  ([#1490](#1490)) ([67662b2](67662b2))
* feeds page typography and styles improvements ([#1467](#1467)) ([a3b289e](a3b289e))
* on page reload Insights page redirecting to Dashboard ([#1517](#1517)) ([397c36e](397c36e))
* update environment variable for Sentry ([#1521](#1521)) ([56ac14b](56ac14b))
* update release workflow to use GitHub app for semantic versioning ([#1484](#1484)) ([3f1ce84](3f1ce84))
* update user interest logo error for machine learning ([#1474](#1474)) ([a286eda](a286eda))
* uses session to update user info for notifications check ([#1486](#1486)) ([60d787e](60d787e))
@takanome-dev takanome-dev deleted the 1444-show-all-notifications branch August 8, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: keep the list of notifications for some time after they have been viewed
4 participants