-
Notifications
You must be signed in to change notification settings - Fork 129
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(core): update Notifications to latest design #12476
Conversation
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Visit the preview URL for this PR (updated for commit 41e053b): https://fundamental-ngx-gh--pr12476-feat-notifications-l-lz5yn6lx.web.app (expires Sat, 05 Oct 2024 13:34:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
8552747
to
60c2862
Compare
60c2862
to
449e963
Compare
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.
Looks good, couple of comments on things that need to be prepared for translations. Looks like most of the directives/components leave it to the application developer to handle all the logic so there is not much to test here. But it is a lot of breaking changes so I hope we don't have too many devs using the previous implementation
* aria-label attribute for the element | ||
* Default is set to 'Notifications' | ||
*/ | ||
ariaLabel = input('Notifications'); |
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 should be prepared for translation
* Title for the group header | ||
* default value: "Expand/Collapse" | ||
*/ | ||
title = input('Expand/Collapse'); |
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.
another one that needs translation
} | ||
} | ||
readonly ariaDescription = computed( | ||
() => `Notification group ${this.groupHeader()?.expanded() ? 'expanded' : 'collapsed'}` |
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.
another in need of translation
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.
everything looks good, just need localization changes as Mike mentioned and thats it. Just in case, i approve it.
449e963
to
41e053b
Compare
Related Issue(s)
closes none
Description
Updated to incorporate the latest fundamental-styles for Notifications.
NOTE: Keyboard interaction for the Notification List is currently missing and will be implemented in a future update.
NOTE: End-to-end tests for Notifications have been removed.
BREAKING CHANGES:
fd-notification-indicator
directive. The Icon component with color is now used for priority/status indicators.Before:
After:
div
with thefd-notification-message-strip-container
directive.Before:
After:
mobile
property has been removed fromfd-notification
, previously used for displaying Notifications in Popover dialogs.fd-notification-popover
directive, which wraps the Notification Group content inside the Popover body.div
with thefd-notification-message-strip
directive applied.expanded
property fromfd-notification-group-header
.fd-notification-group-header-title
directive for the Notification Group title.fd-notification-limit
component. Use the newfd-notification-group-growing-item
component instead.fd-notification-limit-title
directive. Use thefd-notification-group-growing-item-title
directive.fd-notification-limit-description
directive.Before:
After: