Improve notification id #174
Conversation
- _.uniqueId() is not sufficient since the ID is used to persist some data for notifications via session storage. The id generated starts at a low number & increments w/o randomness. - Using _.uniqueId() along with Date.now() adds a random value that cannot be replicated across sessions/refresh
| @@ -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(); | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Renaming the field will touch dozens of files across all 3 repositories :/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
works for me! i can open a quick PR to swap this again. 😄
for notifications via session storage. The id generated starts at a low
number & increments w/o randomness.
be replicated across sessions/refresh
Example: with uniqueIds like
notification_1,notification_2it is relatively easy for a notification to be markedread, but following arefreshhavingreadbe applied to a completely different notification.Needed for Notification Drawer updates web console PR 2001.
@spadgett @jeff-phillips-18