-
Notifications
You must be signed in to change notification settings - Fork 398
Adding multiple item boxes support #633
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
base: main
Are you sure you want to change the base?
Conversation
Itembox notifications can show in a row of up to 5 notifications at once
html/app.js
Outdated
|
|
||
| if (existingIndex > -1) { | ||
| // Update existing notification amount | ||
| this.notifications[existingIndex].amount += notification.amount; |
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.
There's a thing called "DRY" in Programming (Don't repeat yourself).
I would recomment using it like this:
const notification = this.notifications[existingIndex];
notification.amount += .....
/*ALL THE STUFF */
this.notifications[existingIndex] = notification
html/app.js
Outdated
| const index = this.notifications.findIndex(n => n.id === notificationId); | ||
| if (index > -1) { | ||
| // Clear timeout if it exists | ||
| if (this.notifications[index].timeoutId) { |
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.
You get the object 2 times, but you could just get it one time by assigning it to an object:
const timeOutId = this.notifications[index].timeoutId;
if (timeOutId) {
clearTimeout(timeOutId)
}
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.
Thanks for the review and recommendation, I'll look out for that more often
I also updated per recommendation
Recommendation from a reviewer
| // Update existing notification amount | ||
| this.notifications[existingIndex].amount += notification.amount; | ||
| this.notifications[existingIndex].timestamp = Date.now(); | ||
| const existingNotification = this.notifications[existingIndex]; |
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.
You forgit to update the object in notifications-array. JavaScript does not provide pointer's, so you have to update the object in the array again.
like this.notifications[existingIndex] = existingNotification
Itembox notifications can show in a row of up to 5 notifications at once
Normally if it receives 2 or more removal or add events of different items it will only show 1 item box notification instead of all
Checklist