[40147] Snackbar/toast informing users of new notifications has the wrong icon #14977
Conversation
…otifications-has-the-wrong-icon
…e-wrong-icon' of https://github.com/opf/openproject into 40147-toast-informing-users-of-new-notifications-has-the-wrong-icon
There was a problem hiding this comment.
Although that works, I think the solution is a bit too much. It feels weird to introduce a new type because we want to change the icon. I don't want to add new types whenever I want a new icon on a toast. I left some comments, maybe there is a way around that.
Further, please rebase the branch to the dev branch.
| @@ -57,6 +57,7 @@ $nm-pb-regress-color: whiteSmoke | |||
| $nm-pb-regress-fill-color: #00558b | |||
| $nm-pb-border-radius: rem-calc(6) | |||
| $nm-pb-height: rem-calc(20) | |||
| $nm-pb-background-color: #155282 | |||
There was a problem hiding this comment.
I think the name is a bit unfortunate, as pb stands for "progress" and this is not a progress toast.
| @@ -204,6 +217,9 @@ $nm-upload-box-padding: rem-calc(15) rem-calc(25) | |||
| &.-info | |||
| color: $nm-color-info-icon | |||
|
|
|||
| &.-notification | |||
There was a problem hiding this comment.
Is that section really needed?
| @@ -41,7 +41,7 @@ export function removeSuccessFlashMessages():void { | |||
| jQuery('.op-toast.-success').remove(); | |||
| } | |||
|
|
|||
| export type ToastType = 'success'|'error'|'warning'|'info'|'upload'|'loading'; | |||
| export type ToastType = 'success'|'error'|'warning'|'info'|'upload'|'loading'|'notification'; | |||
There was a problem hiding this comment.
I think the name is a bit overloaded. In our system, a notification is something different than a toast. In my eyes, it is kind of a bad style to introduce a whole new type, just to change the icon. Could you please check whether you can simply use the info type and pass an additional class to it, which overrides the icon?
bsatarnejad
force-pushed
the
40147-toast-informing-users-of-new-notifications-has-the-wrong-icon
branch
from
1d139d0 to
f0c0538
Compare
Generated by 🚫 Danger |
bsatarnejad
force-pushed
the
40147-toast-informing-users-of-new-notifications-has-the-wrong-icon
branch
from
1b14ddf to
1894812
Compare
| &.-notification-icon | ||
| &::before | ||
| @include messages-icon | ||
| @extend %nm-icon-notification | ||
|
|
There was a problem hiding this comment.
I think you are over complicating things here. By creating a specific -{{icon}}-icon class, you have to add a CSS rule for every possible icon that you want to pass. Further, the modifier for an icon should only change the icon itself and not the whole toast. Why don't you simply use the classes we already have to render icons?The -infoclass is still set on the toast, so there is no need for the syling if you adapt the rules above so thattoast-icon` also only affects the icon and not the styling.
| @@ -72,6 +72,10 @@ $nm-upload-box-padding: rem-calc(15) rem-calc(25) | |||
| @include icon-mixin-checkmark | |||
| color: $nm-color-success-icon | |||
|
|
|||
| %nm-icon-notification | |||
| @include icon-mixin-bell | |||
| color: $nm-pb-background-color | |||
There was a problem hiding this comment.
There is a semantic mismatch here. It is kind of a bad style to use the variable for the progress toast for the notification toast as we cannot assure that these will always be of the same color. However, regarding my other ocmment, I think we can remove this whole block altogether.
bsatarnejad
deleted the
40147-toast-informing-users-of-new-notifications-has-the-wrong-icon
branch
https://community.openproject.org/work_packages/40147/activity