Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve notification id #174

Merged
merged 1 commit into from
Sep 7, 2017
Merged

Improve notification id #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dist/origin-web-common-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2149,7 +2149,7 @@ angular.module('openshiftCommonUI').provider('NotificationsService', function()
};

var addNotification = function (notification) {
notification.id = notification.id || _.uniqueId('notification-');
notification.id = notification.id || _.uniqueId('notification-') + Date.now();
notification.timestamp = new Date().toISOString();
if (isNotificationPermanentlyHidden(notification) || isNotificationVisible(notification)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion dist/origin-web-common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5720,7 +5720,7 @@ angular.module('openshiftCommonUI').provider('NotificationsService', function()
};

var addNotification = function (notification) {
notification.id = notification.id || _.uniqueId('notification-');
notification.id = notification.id || _.uniqueId('notification-') + Date.now();
notification.timestamp = new Date().toISOString();
if (isNotificationPermanentlyHidden(notification) || isNotificationVisible(notification)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion dist/origin-web-common.min.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2415,7 +2415,7 @@ this.dismissDelay = 8e3, this.autoDismissTypes = [ "info", "success" ], this.$ge
var notifications = [], dismissDelay = this.dismissDelay, autoDismissTypes = this.autoDismissTypes, notificationHiddenKey = function(notificationID, namespace) {
return namespace ? "hide/notification/" + namespace + "/" + notificationID :"hide/notification/" + notificationID;
}, addNotification = function(notification) {
notification.id = notification.id || _.uniqueId("notification-"), notification.timestamp = new Date().toISOString(), isNotificationPermanentlyHidden(notification) || isNotificationVisible(notification) || (notifications.push(notification), $rootScope.$emit("NotificationsService.onNotificationAdded", notification));
notification.id = notification.id || _.uniqueId("notification-") + Date.now(), notification.timestamp = new Date().toISOString(), isNotificationPermanentlyHidden(notification) || isNotificationVisible(notification) || (notifications.push(notification), $rootScope.$emit("NotificationsService.onNotificationAdded", notification));
}, hideNotification = function(notificationID) {
notificationID && _.each(notifications, function(notification) {
notification.id === notificationID && (notification.hidden = !0);
Expand Down
2 changes: 1 addition & 1 deletion src/ui-services/notificationsService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ angular.module('openshiftCommonUI').provider('NotificationsService', function()
};

var addNotification = function (notification) {
notification.id = notification.id || _.uniqueId('notification-');
notification.id = notification.id || _.uniqueId('notification-') + Date.now();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjaminapetersen This has already merged, but I realize we can't use notification.id if set by the caller for track by because it won't always be unique. For some messages, we purposely reuse notification.id so they don't get toasted twice.

We probably need to always generated some of unique ID that is set by the service and is always unique. So this might need to be.

notification.uniqueID = _.uniqueId('notification-') + Date.now();

Then we can use that always for track by. This will also fix an animation issue I've noticed with the toast that the slide in animation is reset for already-visible items when new items are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the notification drawer only needs this uniqueID, correct?

I'm wondering if we can rename the filed that gets reused so we don't double toast. I feel like id and uniqueID is a bit confusing. I imagine this creates more work for us elsewhere.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to use it for track by in the toast notifications as well. We have some animation quirks with the toasts because track by isn't correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the field will touch dozens of files across all 3 repositories :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boo...

So we provide uniqueID and always use that for track by. If the originator of the notification does not set id, we can safely still use the same generated random id for it (like here). So id and uniqueID can match if its irrelevant, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's too terrible if we give the new field a very specific name, maybetrackByID, and a comment that it's there for uniqueness. What do you think?

Yeah, we should always use the unique ID for track by so avoid duplicates in a repeater error. I wouldn't worry about setting notification.id if the caller didn't set one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me! i can open a quick PR to swap this again. 😄

notification.timestamp = new Date().toISOString();
if (isNotificationPermanentlyHidden(notification) || isNotificationVisible(notification)) {
return;
Expand Down