List only 1 'syncing to chain' ntfn #458
Conversation
src/action/notification.js
Outdated
| handlerLbl: handlerLbl || (err ? 'Show error logs' : null), | ||
| display: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This action should probably not need to be changed. Either we call notification.display only once in the info action (problem is we want the notification bar to stay visible, so this won't work) or we filter the notification list to render the syncing info only once. Both aren't really solutions I'm happy with. Thoughts?
There was a problem hiding this comment.
@tanx I think it would make sense to modify computedNotifications to filter the notification list such that Syncing to chain... notifications are limited to once every 5 minutes . I'd also be fine with only showing it once. what do you think?
There was a problem hiding this comment.
Showing it once is fine. Filtering in computedNotifications makes sense. Something like...
const all = [];
notifications.forEach(n => {
if (n.waiting && all.find(a => a.waiting)) {
return;
}
all.push(n);
});
There was a problem hiding this comment.
We only use the waiting flag for syncing so that should do the job.
tanx
left a comment
tanx
left a comment
There was a problem hiding this comment.
@valentinewallace I reverted the previous change and filtered in ComputedNotification like we discussed. Good to merge from my side. Let me know what you think.
| return; | ||
| } | ||
| all.push(n); | ||
| }); |
There was a problem hiding this comment.
This displays only the first waiting notification.
| }), | ||
| notificationCountLabel: computed(() => | ||
| formatNumber(store.computedNotifications.length) | ||
| ), |
There was a problem hiding this comment.
Also moved this here to be computed based on the computedNotifications list size.
valentinewallace
force-pushed
the
list-single-syncing-ntfn
branch
from
0854f9c to
4d9438f
Compare
| date: new Date(1528703821406), | ||
| display: true, | ||
| }); | ||
| store.notifications.push({ |
There was a problem hiding this comment.
Went in to add this test, I see you've already added it 💯
|
Everything great, tested packaged app & looks good :) I rebased onto master, I think that's still OK to merge? Made sure to test it out & everything. |
Closes #456