-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat: destroy-all function in stacked notification #2289
Conversation
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🔴 | Statements | 3.13% (-0% 🔻) |
117/3740 |
🔴 | Branches | 3.56% (+0% 🔼) |
87/2447 |
🔴 | Functions | 1.53% (+0% 🔼) |
19/1240 |
🔴 | Lines | 3.08% (-0% 🔻) |
113/3667 |
Test suite run success
32 tests passing in 4 suites.
Report generated by 🧪jest coverage report action from cb51e3a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about displaying Destroy All
button when only destroyAll
is not undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agatha197 Resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -18,7 +18,8 @@ const BAINotificationItem: React.FC<{ | |||
notification: NotificationState, | |||
) => void; | |||
showDate?: boolean; | |||
}> = ({ notification, onClickAction, showDate }) => { | |||
destroyAll?: () => void | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop
is function type, it's better to use onClickDestroyAll
name patter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
resources/i18n/en.json
Outdated
@@ -1276,7 +1276,8 @@ | |||
"Notifications": "Notifications", | |||
"NoNotification": "No Notification.", | |||
"ClearNotifications": "Clear Notifications", | |||
"AreYouSureToClearAllNotifications": "Are you sure to clear all notifications?" | |||
"AreYouSureToClearAllNotifications": "Are you sure to clear all notifications?", | |||
"DestroyAll": "Destroy All" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the API name is destroy
, How about using Close
instead of Destroy
for a user? @agatha197 @ironAiken2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'clear' is fine, but I think it feels more like 'closing' an open popup, rather than clearing space, so 'close' seems more better. How about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
If antd config provider is provided (codes) and configured via notification.config(), it seems to refer to that config when opening the notification, but it doesn't work properly. Related links : ant-design/ant-design#42799 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR Resolves #2281 Issue
Feature
Checklist: (if applicable)