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

✨ All notifications page #9558

Closed
2 tasks done
Tracked by #9553
vd1992 opened this issue Feb 29, 2024 · 8 comments · Fixed by #9755
Closed
2 tasks done
Tracked by #9553

✨ All notifications page #9558

vd1992 opened this issue Feb 29, 2024 · 8 comments · Fixed by #9755
Assignees
Labels
feature New feature or request.
Milestone

Comments

@vd1992
Copy link
Contributor

vd1992 commented Feb 29, 2024

✨ Feature

All notifications page created

🕵️ Details

  • Update settings links to notification settings on account management page
  • Ignore pinning and filtering for now
  • "..." context menu allows to Mark as read/unread and Delete (delete requires confirmation)
  • Clicking a notification sets it to "read" and navigates to the relevant page (depends on notification type)
  • Mark all as read fires mutation for all user's unread notifications to be marked as read
  • User can toggle between All and Unread notifications
  • Loads all notifications n at a time (n is some hard-coded number). "Load more" loads the next n, like a manually triggered infinite scroll. Repeats until none left to paginate. Does not hide previously loaded notifications.
    • Let's say load 10 at a time for now
  • Delete opens a confirmation dialog:
    image

🎨 Design File

https://www.figma.com/file/bIvDLraUxqGxDBuerPJlNI/Candidates---Notifications?type=design&node-id=813-380&mode=design

@esizer left a note for implementation: https://www.figma.com/file/bIvDLraUxqGxDBuerPJlNI?node-id=876:32811&mode=design#721297727

Component library designs, including hover and focus states: https://www.figma.com/file/guHeIIh8dqFVCks310Wv0G/Style-library?type=design&node-id=2184-5048&mode=design&t=Liav3sHrKuf2RNtr-0

🧑‍🎨 Designer

@substrae

📸 Screenshot

Image

Implementation Suggestions

Each notification from the backend doesn't necessarily come with descriptive text or a link. We should have some utility file for Notifications which can generate the frontend text and link for each type of notification. As we add more types of notifications, we can just add to this utility file.

✅ Acceptance Criteria

  • Page created (at /applicant/notifications)
  • On page load, loads n most recent notifications (n=10)
  • Unread notifications appear different
  • "Load more" button loads n more notifications and adds them to list without reloading page or hiding previously fetched
  • "Mark all as read" button is functional
  • Clicking a notification sets it to "read" (if unread) and links to related page. (not opening a new tab)
  • Text on notification should not be formatted as link - hover state should indicate it is clickable
  • Clickable area is a link in html, though it will need to intercept the click action
  • If "Unread" filter is active, then only unread notifications are loaded
  • Clicking the "..." opens a small context menu which allows for marking the notification as read/unread (depending on current status) or Delete
  • Clicking Delete notification opens a confirmation dialog
  • Marking a notification as read/unread or deleting it should immediately be reflected in the UI
  • "..." menu has an aria label: Manage this [TYPE/SPECIFIC CONTEXT] notification (TBD)

🛑 Blockers

Issues which must be completed before this one.

Blocked By

Preview Give feedback
  1. feature
    esizer
  2. 0 of 1
    deployment feature
    esizer
@vd1992 vd1992 added the feature New feature or request. label Feb 29, 2024
@vd1992 vd1992 added this to the Notifications milestone Feb 29, 2024
Copy link

github-actions bot commented Mar 1, 2024

Status: Ready to merge ✔️

Issues blocking this PR:


This comment was automatically written by the Blocking Issues bot, and this PR will be monitored for further progress.

@github-actions github-actions bot added the blocked: dependencies Blocked by other issues. label Mar 1, 2024
@tristan-orourke tristan-orourke added the review in refinement Ready to be looked at and pulled into "ready to dev" label Mar 8, 2024
@tristan-orourke tristan-orourke moved this to 🏭 Refinery in GC Digital Talent Mar 11, 2024
@tristan-orourke
Copy link
Member

tristan-orourke commented Mar 11, 2024

@substrae

  • can we change page title from "All notifications" to "Notifications"?
  • what is the aria label for the "..." button
  • how many notifications should we load at a time?

@tristan-orourke
Copy link
Member

Questions:

  • how can we mark a notification as read when its clicked, if its a link?
  • Are these cards meant to have a hover state?
  • should the text be formatted as a link?
  • should parts of the text be formatted as a link?

    Your status changed to Qualified for the IT-01: pool name process

@tristan-orourke tristan-orourke removed the review in refinement Ready to be looked at and pulled into "ready to dev" label Mar 11, 2024
@JoshBeveridge
Copy link
Member

JoshBeveridge commented Mar 12, 2024

@tristan-orourke

  • Sure, though I'm curious as to why?
  • Manage this [TYPE/SPECIFIC CONTEXT] notification - I'm going to leave this up to @RM-1978 and @esizer to finalize, because I'm not sure what the exact best approach is; we don't necessarily want to repeat the entire notification in the aria-label, so there's going to be some finessing to get it to the point where it is specific enough without being cumbersome.
  • This is something y'all should decide based on what's reasonable load-limit wise. My thought is that we have at least 10-20.
  • I'm assuming this isn't a question for me and more of a technical thought about implementation (though could you not interrupt the link behaviour, capture the read state, and then continue the link behaviour?)
  • Yes to the hover state (they should also have a focus state, I'll add both to Figma).
  • Formatting full sentences as links is bad practice and generally makes text illegible.
  • This might be possible, but we've included instructions on the fact that the notifications are clickable AND they'll have hover states; most notification panes act this way, so I'm not worried about the lack of underline.

@JoshBeveridge
Copy link
Member

JoshBeveridge commented Mar 12, 2024

Figma has been updated with focus and hover states for both types of notifications, as well as their action menus. I've also nudged the position of the action menu icon in the card style to account for a larger clickable area and visual balance during focus/hover.

@tristan-orourke
Copy link
Member

@substrae

Why "All notifications" to "Notifications"

Just consistency, I think. We don't have the word "All" in most page titles.

I'm assuming this isn't a question for me and more of a technical thought about implementation

Yes, that wasn't for you, sorry 😆

@tristan-orourke
Copy link
Member

This is something y'all should decide based on what's reasonable load-limit wise. My thought is that we have at least 10-20.

For now I'll just make the call to load 10 at a time. We can change it easily if that makes sense.

@tristan-orourke tristan-orourke added the review in refinement Ready to be looked at and pulled into "ready to dev" label Mar 13, 2024
@tristan-orourke
Copy link
Member

Aria label for context menu can be delegated to individual notification

@tristan-orourke tristan-orourke removed the review in refinement Ready to be looked at and pulled into "ready to dev" label Mar 13, 2024
@tristan-orourke tristan-orourke moved this from 🏭 Refinery to 🏃 Ready to Develop in GC Digital Talent Mar 13, 2024
@esizer esizer self-assigned this Mar 13, 2024
@esizer esizer moved this from 🏃 Ready to Develop to 🏗 In progress in GC Digital Talent Mar 13, 2024
@github-actions github-actions bot removed the blocked: dependencies Blocked by other issues. label Mar 13, 2024
@esizer esizer moved this from 🏗 In progress to 👀 In review in GC Digital Talent Mar 14, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in GC Digital Talent Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants