Skip to content

change: [M3-7873] - Use Chip for notification badge - #10333

Merged
hana-akamai merged 2 commits into
linode:developfrom
hana-akamai:M3-7873-improve-notification-count-display
Mar 29, 2024
Merged

hana-akamai merged 2 commits into
linode:developfrom
hana-akamai:M3-7873-improve-notification-count-display

Conversation

@hana-akamai

Copy link
Copy Markdown
Contributor

Description 📝

When the Notification count is past 100, the numbers become difficult to read because their edges appear outside the colored dot. I decided to use the chip component to better display the Notification count.

Preview 📷

Before After
image image

How to test 🧪

Verification steps

(How to verify changes)

  • Update the label to a 3 digit number on line 141 of NotificationMenu.tsx
  • Confirm the notification badge display in the top right corner

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@hana-akamai hana-akamai self-assigned this Mar 29, 2024
@hana-akamai
hana-akamai requested a review from a team as a code owner March 29, 2024 16:09
@hana-akamai
hana-akamai requested review from carrillo-erik and hkhalil-akamai and removed request for a team March 29, 2024 16:09

@bnussman-akamai bnussman-akamai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks much better!

@@ -135,7 +135,12 @@ export const NotificationMenu = () => {
<NotificationIconWrapper isMenuOpen={notificationContext.menuOpen}>
<Bell />
{numNotifications > 0 ? (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

&&

@mjac0bs mjac0bs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice change! Confirmed things look good at desktop/tablet/mobile viewports for various #s of notifications.

We do see the chip display next, instead of overlapping, to the notification icon at screens < 370 wide, but that's the case in prod now even with the circular badge icon, so I don't think we generally support screens that small.

Single digits 3 or more digits
Screenshot 2024-03-29 at 9 15 27 AM Screenshot 2024-03-29 at 9 20 16 AM

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Mar 29, 2024
@github-actions

github-actions Bot commented Mar 29, 2024 •

Copy link
Copy Markdown

Coverage Report: ✅
Base Coverage: 81.72%
Current Coverage: 81.72%

@jdamore-linode

Copy link
Copy Markdown
Contributor

Such a little thing but this has bugged me forever! Thanks @HanaXu

@hkhalil-akamai hkhalil-akamai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is something so satisfying about the rounded rectangle 🤩

@hana-akamai
hana-akamai merged commit f08356f into linode:develop Mar 29, 2024
@hana-akamai
hana-akamai deleted the M3-7873-improve-notification-count-display branch March 29, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants