Add Notification Feed for per-course-Alerts#414
Conversation
ribhavsharma
changed the title
|
draft-ing for now, will fix build issues |
packages/frontend/app/(dashboard)/course/[cid]/queue/[qid]/components/TAQuestionCardButtons.tsx
Show resolved
Hide resolved
| </div> | ||
| <div className="flex items-center"> | ||
| <NotificationBell /> | ||
| </div> |
There was a problem hiding this comment.
I think the bell on mobile should be moved since it makes the page title/subtitle off-centre (also, for long course/queue names or on phones with increased text size, it may cause the title text to overflow/wrap easier which looks really bad)
Proposed solution:
Place an antd Badge on the hamburger menu itself
Place the notification bell somewhere in this drawer (though ngl I'm having some trouble thinking of an area to place it. Can't place it at the top since the university name can be longer. Can't place it beside the user's name since their name can be longer)
one option
Perhaps we can place it in-between the profile and logout buttons, similar to the desktop solution I proposed)
But once the user clicks on it... I'm not sure what should happen, all the options I can think of are kinda gross:
- Have a submenu (similar to the Queues dropdown). However, it can't actually drop downwards due to the lack of space beneath so the menu, so it would need to drop "upwards" which feels kinda jank. Also it'd be really easy to accidentally close this notifications submenu by accidentally tapping, thus requiring you to re-open the hamburger menu and then re-open the notifications menu. Overall though, this isn't a terrible option i think
- Open a modal (kinda yucky imo, i think it would have higher cognitive load, but it could at least be re-used if the desktop also uses a modal for notifications)
another option
Just say frick it and fit the notification bell up top where the university name is.
This might get kinda iffy for future orgs, but at least now you can make a proper dropdown submenu.
It is also a little more hidden up there imo so people might have a little more trouble finding it (at least in OC's case where the OC logo has a similar colour to the red "1" for notifications), but overall I think this is the better option.
There was a problem hiding this comment.
i agree with hiding it in the mobile sidebar menu thing though bc of the lack of usable space on mobile
There was a problem hiding this comment.
this is still not resolved
While it doesn't look that terrible, I have a few issues with it:
- The bell and hamburger menu are very close together, meaning it's gonna be easy to accidentally tap either one.
- It makes the course title off-centre (small)
- Lack of space
So I think it should be moved into the navigation menu beside the organization logo, as stated in "another option" above. Also don't forget to place the Badge on the hamburger icon too
| async getAllAlerts( | ||
| @UserId() userId: number, | ||
| @Query('mode') mode?: string, | ||
| @Query('includeRead') includeRead?: string, |
There was a problem hiding this comment.
I would use nestjs's integrated validation stuff for the query params. https://react.dev/reference/react/useContext
i.e. ParseBoolPipe, parseEnumPipe
| @IsString() | ||
| @IsOptional() | ||
| subtype?: | ||
| | 'commentOnMyPost' | ||
| | 'commentOnOthersPost' | ||
| | 'humanAnswered' | ||
| | 'statusChanged' | ||
| | 'upvoted' |
There was a problem hiding this comment.
this (and all references to it) should 100% be put into an enum
| </div> | ||
| <div className="flex items-center"> | ||
| <NotificationBell /> | ||
| </div> |
There was a problem hiding this comment.
this is still not resolved
While it doesn't look that terrible, I have a few issues with it:
- The bell and hamburger menu are very close together, meaning it's gonna be easy to accidentally tap either one.
- It makes the course title off-centre (small)
- Lack of space
So I think it should be moved into the navigation menu beside the organization logo, as stated in "another option" above. Also don't forget to place the Badge on the hamburger icon too
| const markAllReadLocal = async () => { | ||
| await markAllRead() | ||
| } |
There was a problem hiding this comment.
This should probably include some kind of .catch() to alert the user if it fails. Also changes will need to be made to markAllRead inside alertsContext to allow this
| const markAllRead = async () => { | ||
| // optimistic: clear all and set total 0 | ||
| mutate( | ||
| (currentPages: GetAlertsResponse[] | undefined) => | ||
| currentPages | ||
| ? currentPages.map((p, idx) => ({ | ||
| ...p, | ||
| alerts: [], | ||
| total: idx === 0 ? 0 : p.total, | ||
| })) | ||
| : currentPages, | ||
| { revalidate: false }, | ||
| ) | ||
| try { | ||
| await API.alerts.markReadAll() | ||
| } finally { | ||
| await mutate(undefined, { revalidate: true }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This currently does not handle error cases. Probably the easiest way to do this is to add a catch that either: returns a Promise.reject that you then handle in subcomponents that use this method OR you try just putting a message.error("Error marking all alerts as read: {getErrorMessage(err)}") inside of this context directly, but a part of me thinks that might error for whatever reason.
Same goes for markRead above, and closeCourseAlert below
| }, [currentPageAlerts]) | ||
|
|
||
| const { data: courseResponses } = useSWR( | ||
| uniqueCourseIds.length > 0 ? ['courses-for-alerts', uniqueCourseIds] : null, |
There was a problem hiding this comment.
2 things with this:
- Adding a SWR call (or anything that does an API call on component mount, such as a useEffect that does an API call) inside of this component will cause repeated unnecessary API calls since this component may get remounted a lot. For example, once this Bell is inside the HeaderBar navigation drawer (which still needs to be done), this component will get remounted every time the user taps the hamburger icon to open the nav menu. This is one if the big advantages of using an alertsContext inside a layout.tsx, since now that SWR call is located in a higher "component"; where even if this NotificationBell gets remounted a dozen times, the SWR call still only happens once. Or in summary: By keeping API calls that get data and their corresponding state variables stored in higher components, it allows us to use the data in lower components repeatedly without duplicate/unnecessary API calls.
(I should also clarify that "SWR call" isn't like a proper term, I just mean using useSWR which is just an API call followed by many more API calls to keep revalidating the data to make sure it's up-to-date)
- All of the courses (including the course names) are stored and readily available inside the userInfo context :)
That way, you can remove "uniqueCourseIds" and "courseResponses" and "courseNameMap" and just use userInfo.courses
| payload.subtype === 'commentOnMyPost' | ||
| ? 'New comment on your Anytime Question' | ||
| : payload.subtype === 'commentOnOthersPost' | ||
| ? 'New comment on a followed Anytime Question' |
There was a problem hiding this comment.
| ? 'New comment on a followed Anytime Question' | |
| ? 'New comment an Anytime Question you commented on' |
Don't like using the term "follow" here since people might think they can "unfollow" it
| courseId: question.courseId, | ||
| questionId: question.id, | ||
| subtype: 'humanAnswered', | ||
| summary: 'Your Anytime Question has been answered', |
There was a problem hiding this comment.
| summary: 'Your Anytime Question has been answered', | |
| summary: 'New Answer: "{answerText}"', |
| : payload.subtype === 'commentOnOthersPost' | ||
| ? 'New comment on a followed Anytime Question' | ||
| : payload.subtype === 'humanAnswered' | ||
| ? 'Your Anytime Question was answered' |
There was a problem hiding this comment.
| ? 'Your Anytime Question was answered' | |
| ? 'Your Anytime Question was answered by staff' |
| console.error('Failed to send email Vote Question email: ' + err); | ||
| Sentry.captureException(err); | ||
| }); | ||
| await AlertModel.create({ |
There was a problem hiding this comment.
this is also still inside the subscription.
| courseId, | ||
| payload, | ||
| targetUserId: question.creatorId, | ||
| deliveryMode: AlertDeliveryMode.FEED, |
There was a problem hiding this comment.
| deliveryMode: AlertDeliveryMode.FEED, | |
| deliveryMode: AlertDeliveryMode.MODAL, |
3 participants
Description
Added a notification feed on the header bar, using REPHRASE_QUESTION and Anytime Question updates as examples.
Extended the existing Alert infrastructure, adding a
deliveryModefield to switch between modals, and the notification field.Closes #356
Type of change
yarn installHow Has This Been Tested?
Tested manually using interactions between the admin user and studentOne.
Checklist:
console.logs, leftover unused logic, or anything else that was accidentally committed)Demo
notif.mp4