Navbar: Add Badge icon to Notification Button #2424

ElishaSamPeterPrabhu · 2024-04-09T10:28:03Z

Description

Added prop counter as a badge to the notification icon button

References #
Fixes: #1011

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

How Has This Been Tested?

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

netlify · 2024-04-09T10:28:18Z

✅ Deploy Preview for moduswebcomponents ready!

Name	Link
🔨 Latest commit	`a4b303a`
🔍 Latest deploy log	https://app.netlify.com/sites/moduswebcomponents/deploys/6618890362b1e800083caa52
😎 Deploy Preview	https://deploy-preview-2424--moduswebcomponents.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 27 (no change from production) Accessibility: 98 (no change from production) Best Practices: 92 (no change from production) SEO: 92 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

cjwinsor · 2024-04-09T17:43:53Z

stencil-workspace/src/components/modus-navbar/modus-navbar.tsx

@@ -70,6 +70,9 @@ export class ModusNavbar {
  /** (optional) Whether to show notifications. */
  @Prop() showNotifications: boolean;

+  /** (optional) Whether to show badge on top of notification */
+  @Prop() counter: string;


Can we make this notificationCount as a number?

Isn't string preferable so in the future we can allow up to 3 character string

Made it to be a number for now

cjwinsor · 2024-04-09T17:49:43Z

stencil-workspace/src/components/modus-navbar/modus-navbar.tsx

+        this.counter = null;
+      }
+    }
+    console.log('countervalue', counterValue);


Remove console logs

Also this above logic can probably be simplified if the prop is a number

Also break this into a separate function, render is getting a little heavy with all this logic. something like

const notificationCount = getNotificationCount()`

That can return a string value

cjwinsor · 2024-04-09T17:50:11Z

stencil-workspace/storybook/stories/components/modus-navbar/modus-navbar.stories.tsx

@@ -109,14 +115,16 @@ const defaultApps = [{
  url: 'https://modus.trimble.com/',
 }];

-const Template = ({ buttons, enableSearchOverlay, helpTooltip, profileMenuOptions, searchTooltip, showHelp, showProfile, showSearch }) => html`
+const Template = ({ buttons,counter, enableSearchOverlay, helpTooltip, profileMenuOptions, searchTooltip, showHelp, showProfile, showSearch }) => html`


missing a space after the , and before counter

cjwinsor · 2024-04-09T17:50:30Z

stencil-workspace/storybook/stories/components/modus-navbar/modus-navbar.stories.tsx

 };

-const FailedToLoadAvatarTemplate = ({ buttons, enableSearchOverlay, helpTooltip, profileMenuOptions, searchTooltip, showHelp, showProfile, showSearch }) => html`
+const FailedToLoadAvatarTemplate = ({ buttons,counter, enableSearchOverlay, helpTooltip, profileMenuOptions, searchTooltip, showHelp, showProfile, showSearch }) => html`


missing a space after the , and before counter

cjwinsor · 2024-04-09T17:51:36Z

@coliff @egunther39 Can you take a look and let me know if the size of this notification badge is too small in the deploy preveiw?...

cjwinsor · 2024-04-09T17:53:10Z

stencil-workspace/src/components/modus-navbar/modus-navbar.vars.scss

@@ -58,3 +58,6 @@ $modus-navbar-blue-profile-icon-active-border-color: var(
  --modus-navbar-blue-profile-icon-active-border-color,
  #019aeb
 ) !default;
+
+// Badge
+$modus-navbar-badge-bg: var(--modus-navbar-badge-bg, #da212c) !default;


Badge is showing as blue in deploy preview

egunther39 · 2024-04-09T18:03:28Z

@cjwinsor - It's a bit small. Here is an example of the red badge we use in our application. The badge is 20x20 inside/overlaid above the 48x48 navbar button/icon. I'd suggest, doing the same.

cjwinsor · 2024-04-09T18:14:39Z

Here is a shot with a medium badge

cjwinsor · 2024-04-10T21:26:15Z

stencil-workspace/src/components/modus-navbar/modus-navbar.tsx

@@ -349,6 +352,21 @@ export class ModusNavbar {
    }
  }

+  getNotificationCount(): string {
+    let counterValue;
+    if (this.notificationCount) {


To avoid nesting, we can treat this as a guard statement

if (!this.notificationCount) { return; }

Also I don't do one liners for this because I feel they are slightly less readable.

Updated code without one liners and avoiding nesting

cjwinsor · 2024-04-10T21:26:50Z

stencil-workspace/storybook/stories/components/modus-navbar/modus-navbar-storybook-docs.mdx

@@ -223,6 +223,7 @@ interface ModusNavbarButton {
 | ---------------------- | ------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------- | ----------- |
 | `apps`                 | --                       | (optional) The apps to render in the apps menu.                                                                                                                                         | `ModusNavbarApp[]`        | `undefined` |
 | `buttons`              | --                       | (optional) The buttons to render in the Navbar.                                                                                                                                         | `ModusNavbarButton[]`     | `undefined` |
+| `notificationCount`    | `notification-count`     | (optional) To add the counter value to the notification icon.                                                                                                                           | `string`                  | `undefined` |


This should be a number now, not a string

cjwinsor · 2024-04-10T21:29:06Z

stencil-workspace/storybook/stories/components/modus-navbar/modus-navbar.stories.tsx

+      name: 'notification-count',
+      description: 'To add the counter value to the notification icon',
+      table: {
+        type: { summary: 'string' },


This should be handled as a number.

cjwinsor · 2024-04-10T21:29:58Z

stencil-workspace/storybook/stories/components/modus-navbar/modus-navbar.stories.tsx

@@ -174,16 +184,18 @@ Default.args = {
  showHelp: false,
  showProfile: true,
  showSearch: false,
+  notificationCount:""


All these can be 0. I'm not sure how storybook handles undefined or null when it comes to showing the control. This applies to all 3 stories.

cjwinsor · 2024-04-10T21:36:01Z

@ElishaSamPeterPrabhu Notifications are still blue in the deploy preview as well.

ElishaSamPeterPrabhu · 2024-04-11T13:21:42Z

@ElishaSamPeterPrabhu Notifications are still blue in the deploy preview as well.

Updated to red

Update badge icon in notification button

1b607eb

ElishaSamPeterPrabhu requested a review from coliff as a code owner April 9, 2024 10:28

ElishaSamPeterPrabhu changed the title ~~Update badge icon in notification button~~ Navbar: Add Badge icon to Notification Button Apr 9, 2024

Merge branch 'main' into issue-1011/notification-button-badge

9cc6ca7

cjwinsor requested changes Apr 9, 2024

View reviewed changes

cjwinsor reviewed Apr 9, 2024

View reviewed changes

ElishaSamPeterPrabhu and others added 2 commits April 10, 2024 10:11

Merge branch 'main' into issue-1011/notification-button-badge

780d175

Update code

421ef2c

cjwinsor requested changes Apr 10, 2024

View reviewed changes

ElishaSamPeterPrabhu and others added 2 commits April 11, 2024 18:45

Format code

acaea9a

Merge branch 'main' into issue-1011/notification-button-badge

49c4437

Badge: Update padding so counter is round

a4b303a

cjwinsor approved these changes Apr 12, 2024

View reviewed changes

cjwinsor added modus-navbar modus-badge labels Apr 12, 2024

cjwinsor merged commit bd30463 into main Apr 12, 2024
9 checks passed

cjwinsor deleted the issue-1011/notification-button-badge branch April 12, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navbar: Add Badge icon to Notification Button #2424

Navbar: Add Badge icon to Notification Button #2424

ElishaSamPeterPrabhu commented Apr 9, 2024 •

edited

Loading

netlify bot commented Apr 9, 2024 •

edited

Loading

cjwinsor Apr 9, 2024

ElishaSamPeterPrabhu Apr 10, 2024

ElishaSamPeterPrabhu Apr 10, 2024

cjwinsor Apr 9, 2024

ElishaSamPeterPrabhu Apr 10, 2024

cjwinsor Apr 9, 2024

ElishaSamPeterPrabhu Apr 10, 2024

cjwinsor Apr 9, 2024

ElishaSamPeterPrabhu Apr 10, 2024

cjwinsor commented Apr 9, 2024 •

edited

Loading

cjwinsor Apr 9, 2024

egunther39 commented Apr 9, 2024

cjwinsor commented Apr 9, 2024

cjwinsor Apr 10, 2024

ElishaSamPeterPrabhu Apr 11, 2024

cjwinsor Apr 10, 2024

ElishaSamPeterPrabhu Apr 11, 2024

cjwinsor Apr 10, 2024

ElishaSamPeterPrabhu Apr 11, 2024

cjwinsor Apr 10, 2024

ElishaSamPeterPrabhu Apr 11, 2024

cjwinsor commented Apr 10, 2024

ElishaSamPeterPrabhu commented Apr 11, 2024 •

edited

Loading

Navbar: Add Badge icon to Notification Button #2424

Navbar: Add Badge icon to Notification Button #2424

Conversation

ElishaSamPeterPrabhu commented Apr 9, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist

netlify bot commented Apr 9, 2024 • edited Loading

✅ Deploy Preview for moduswebcomponents ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwinsor commented Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

egunther39 commented Apr 9, 2024

cjwinsor commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwinsor commented Apr 10, 2024

ElishaSamPeterPrabhu commented Apr 11, 2024 • edited Loading

ElishaSamPeterPrabhu commented Apr 9, 2024 •

edited

Loading

netlify bot commented Apr 9, 2024 •

edited

Loading

cjwinsor commented Apr 9, 2024 •

edited

Loading

ElishaSamPeterPrabhu commented Apr 11, 2024 •

edited

Loading