fix: (Core) refactor Notifications to follow the latest design specs #5186

InnaAtanasova · 2021-04-22T20:22:42Z

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

refactored Notifications component to reflect the latest css from Fundamental-styles

BREAKING CHANGES:

Changes in markup
Removed fd-notification-avatar directive
The directive fd-notification-actions is now a component
Removed fd-notification-text directive
Removed fd-notification-description directive
Removed fd-notification-metadata directive
Removed the Notifications From Object option together with the NotificationDefault.
Notifications Group is no longer a banner displayed in the right corner but displayed inside a popover triggered by clicking the bell icon in the shell bar at the top right of the screen.

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
Run npm run build-pack-library and test in external application

Documentation checklist:

update README.md
Breaking Changes Wiki
Documentation Examples
Stackblitz works for all examples

netlify · 2021-04-22T20:29:37Z

Deploy preview for fundamental-ngx ready!

Built with commit f30d5ff

https://deploy-preview-5186--fundamental-ngx.netlify.app

InnaAtanasova · 2021-04-23T19:34:14Z

libs/core/src/lib/notification/notification/notification.component.scss

+.fd-notification--group .fd-notification__body:last-child {
+    border-bottom: var(--sapList_BorderWidth) solid var(--sapList_BorderColor);


This is fixed in PR in styles: SAP/fundamental-styles#2272

N1XUS

Great job! I've commented couple of small issues I've noticed.

libs/core/src/lib/action-sheet/action-sheet.component.ts

libs/core/src/lib/notification/notification-group-header/notification-group-header.component.ts

libs/core/src/lib/notification/notification-group-list/notification-group-list.component.ts

N1XUS · 2021-05-02T16:06:19Z

libs/core/src/lib/notification/notification-group/notification-group.component.ts

+    }
+
+    /** @hidden */
+    elementRef(): ElementRef<any> {


Do we need an explicit type argument "any" here?

yes. That's also how it's done in the other components:

But the generic type of ElementRef is any by default, so this code is copying the default behavior

ok...will remove it but it will be inconsistent with the rest of the app. Prob need to create an issue to refactor it for all components.

libs/core/src/lib/notification/notification-service/notification.service.ts

N1XUS · 2021-05-02T16:13:49Z

libs/core/src/lib/notification/notification/notification.component.ts

+            this._subscriptions.add(
+                this._router.events.pipe(
+                    filter(event => event instanceof NavigationStart && this.closeOnNavigation)
+                ).subscribe(event => this.notificationRef.dismiss())


Event here is not used, do we need it in arguments?

InnaAtanasova · 2021-05-03T15:47:44Z

@N1XUS thanks for your review, your comments have been addressed.

mikerodonnell89 · 2021-05-03T21:02:03Z

Notification w/ message strip - notification body has padding-top after message strip is dismissed so it looks a bit asymmetrical vertically

I also noticed that if I open the "component as content" example then open the grouping example, then close the component as content popup, the grouping example closes as well. Might be a problem if multiple notifications are being displayed at once

mikerodonnell89 · 2021-05-03T21:12:52Z

libs/core/src/lib/notification/notification-group-header/notification-group-header.component.ts

+
+    /** @hidden */
+    private _listenRtl(): void {
+        if (this._rtlService) {


This component should also depend on the content density service and set button sizes accordingly

InnaAtanasova · 2021-05-04T13:55:44Z

Notification w/ message strip - notification body has padding-top after message strip is dismissed so it looks a bit asymmetrical vertically

I also noticed that if I open the "component as content" example then open the grouping example, then close the component as content popup, the grouping example closes as well. Might be a problem if multiple notifications are being displayed at once

This is a css issue. The body of the notification that has a message strip has a margin-top.

mikerodonnell89

After investigating, input actually seems to be the best way to deal with message strip margin

…5186) BREAKING CHANGE: Changes in markup Removed fd-notification-avatar directive The directive fd-notification-actions is now a component Removed fd-notification-text directive Removed fd-notification-description directive Removed fd-notification-metadata directive Removed the Notifications From Object option together with the NotificationDefault. Notifications Group is no longer a banner displayed in the right corner but displayed inside a popover triggered by clicking the bell icon in the shell bar at the top right of the screen.

InnaAtanasova added the denoland label Apr 22, 2021

InnaAtanasova requested a review from a team April 22, 2021 20:22

InnaAtanasova self-assigned this Apr 22, 2021

InnaAtanasova force-pushed the feat/implement-new-notifications branch from 80712fa to 2584b5a Compare April 23, 2021 17:27

InnaAtanasova commented Apr 23, 2021

View reviewed changes

droshev requested review from N1XUS and aberikashvili May 2, 2021 15:41

N1XUS suggested changes May 2, 2021

View reviewed changes

mikerodonnell89 requested changes May 3, 2021

View reviewed changes

InnaAtanasova requested review from N1XUS and mikerodonnell89 May 4, 2021 19:59

N1XUS approved these changes May 5, 2021

View reviewed changes

mikerodonnell89 approved these changes May 5, 2021

View reviewed changes

droshev force-pushed the feat/implement-new-notifications branch from 5403733 to abe32bd Compare May 7, 2021 03:01

InnaAtanasova added 10 commits May 7, 2021 06:07

initial commit for notifications

23068d9

add examples

b7428ce

notification group and examples

db22a13

final improvements on the doc

8e56881

add more tests

9353242

fix stupid failing test

ec41782

add router testing module

70355f1

address PR review comments

23a6b12

PR review comments

6a70aff

refactor a bit for message strip

6679d38

droshev force-pushed the feat/implement-new-notifications branch from abe32bd to 6679d38 Compare May 7, 2021 10:07

chore: update package-lock.json

0c59f20

InnaAtanasova force-pushed the feat/implement-new-notifications branch from 0c59f20 to 0aa9056 Compare May 7, 2021 19:48

droshev force-pushed the feat/implement-new-notifications branch from 06b7372 to 0c59f20 Compare May 7, 2021 23:22

update package-lock and remove hasMessage from the docs

f30d5ff

droshev self-requested a review May 10, 2021 14:58

droshev approved these changes May 10, 2021

View reviewed changes

InnaAtanasova merged commit 5d2bf88 into main May 10, 2021

InnaAtanasova deleted the feat/implement-new-notifications branch May 10, 2021 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (Core) refactor Notifications to follow the latest design specs #5186

fix: (Core) refactor Notifications to follow the latest design specs #5186

InnaAtanasova commented Apr 22, 2021

netlify bot commented Apr 22, 2021 •

edited

Loading

InnaAtanasova Apr 23, 2021

N1XUS left a comment

N1XUS May 2, 2021

InnaAtanasova May 3, 2021

N1XUS May 3, 2021

InnaAtanasova May 4, 2021

N1XUS May 2, 2021

InnaAtanasova commented May 3, 2021

mikerodonnell89 commented May 3, 2021

mikerodonnell89 May 3, 2021

InnaAtanasova commented May 4, 2021

mikerodonnell89 left a comment

		.fd-notification--group .fd-notification__body:last-child {
		border-bottom: var(--sapList_BorderWidth) solid var(--sapList_BorderColor);

fix: (Core) refactor Notifications to follow the latest design specs #5186

fix: (Core) refactor Notifications to follow the latest design specs #5186

Conversation

InnaAtanasova commented Apr 22, 2021

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

netlify bot commented Apr 22, 2021 • edited Loading

InnaAtanasova Apr 23, 2021

Choose a reason for hiding this comment

N1XUS left a comment

Choose a reason for hiding this comment

N1XUS May 2, 2021

Choose a reason for hiding this comment

InnaAtanasova May 3, 2021

Choose a reason for hiding this comment

N1XUS May 3, 2021

Choose a reason for hiding this comment

InnaAtanasova May 4, 2021

Choose a reason for hiding this comment

N1XUS May 2, 2021

Choose a reason for hiding this comment

InnaAtanasova commented May 3, 2021

mikerodonnell89 commented May 3, 2021

mikerodonnell89 May 3, 2021

Choose a reason for hiding this comment

InnaAtanasova commented May 4, 2021

mikerodonnell89 left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 22, 2021 •

edited

Loading