-
Notifications
You must be signed in to change notification settings - Fork 9
Notificaion polishing #1538
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
Notificaion polishing #1538
Conversation
…bundle into notificaion-polishing
…bundle into notificaion-polishing
Screencast.from.2025-05-28.12-43-29.webm |
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.
It LGTM - I just added 2 comments, please check them before merging it 👍
assets/js/src/core/modules/notifications/send-notification/send-notification-modal.tsx
Outdated
Show resolved
Hide resolved
assets/js/src/core/modules/user/components/user-select/user-select.tsx
Outdated
Show resolved
Hide resolved
We cannot change this as we would need to also adjust it in the classic bundle. One attachment is fine for this use case.
I tend to agree but let's keep it like it is. At the moment in a lot of situations we do this and it's also done like this in the classic bundle. We would need to think about it in a holistic way - not that important for now... |
…bundle into notificaion-polishing
|
Changes in this pull request
Resolves #
Additional info
Polished Send Notification Modal:
Adjust for backend updates:
[Notifications] - please return fullPath of Attachment rather than just ID studio-backend-bundle#1116
2 Questions / Thoughts:
As in below shot user hovers and sees the tooltip "Open" but icon is greyed out. Cause it is the first Icon it draws the users attention anyway. I think the other icons should be invisible