Replace toastr with Ant Notification #3610
Conversation
ghost
added
the
| export default { // export Ant's notification and replace actions | ||
| ...notification, | ||
| ...updatedActions, | ||
| }; |
There was a problem hiding this comment.
For notification consistency, consider some constants to be used across components:
{
SAVE_SUCCESS: 'Saved',
SAVE_ERROR: 'Failed saving',
...
}notification.error(notification.SAVE_ERROR, 'Sth went wrong');
There was a problem hiding this comment.
This is an interesting idea, but not sure if we really need it. At least, we should check if we have two or more messages with the same titles.
There was a problem hiding this comment.
We do. Many.
And some others are variations which I think is worth consolidating.
There was a problem hiding this comment.
But, it's definitely out-of-scope here. Future thinking.
There was a problem hiding this comment.
We need this for sure 👍. I think for now we can add both SAVE_SUCCESS and SAVE_ERROR as they are a direct change in code and wouldn't be out-of-scope. This will at least define a future pattern and we can later modify existing notifications, WDYT?
There was a problem hiding this comment.
I think it has value for ux, but I'd rather land this as is so we could move on to the next angular2react component.
| @@ -33,18 +34,14 @@ function disableUser(user) { | |||
| return $http | |||
| .post(disableResource(user)) | |||
| .then((data) => { | |||
| toastr.warning(`User <b>${userName}</b> is now disabled.`, { allowHtml: true }); | |||
| notification.warning(`User ${userName} is now disabled.`); | |||
There was a problem hiding this comment.
I dunno if this should be a warning.. maybe notification.info?
There was a problem hiding this comment.
@ranbena in this case it's okay - we show message that user was disabled, and admin should see that they probably did something unsafe
There was a problem hiding this comment.
I thought it annoying with toastr, but with notification it turns out to be okay (that's why I've opted not to change it during the replacement)
IMO both options are suitable here, but if you still think it's better with info I can simply change that @ranbena :)
|
I think the default fixed width is not great. Short messages are too distracting and needlessly bulky
But with flexible width, a bit less
Long texts will break line
But not with flexible width
/* 🎁 */
.ant-notification {
width: auto;
}
.ant-notification-notice {
padding-right: 48px;
} |
gabrieldutra
changed the title
What type of PR is this? (check all applicable)
Description
I've considered using the
Messagecomponent for some cases and I've even given it a try, however as @kravets-levko mentioned in #3547, it's annoying sometimes (even to small messages). Notification seemed a better fit anyway.What this does:
notification.jsto set our defaults to AntNotificationand replace function calls to be more similar to toastr (made life easier to change stuff + I think it will be easier to use);toastrcalls to usenotification;toastrfrom our dependencies 🙂Related Tickets & Documents
Closes #3547
Mobile & Desktop Screenshots/Recordings (if there are UI changes)