Skip to content

Addons Notification polish #515

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

Closed
humitos opened this issue Jan 29, 2025 · 7 comments
Closed

Addons Notification polish #515

humitos opened this issue Jan 29, 2025 · 7 comments
Assignees

Comments

@humitos
Copy link
Member

humitos commented Jan 29, 2025

There is definitely some better UX around notifications we can improve. Especially browsing PR builds, I keep getting the notification, and I think there should be some kind of default behavior that hides it after I've seen it 2-3 times.

@humitos humitos converted this from a draft issue Jan 29, 2025
@humitos
Copy link
Member Author

humitos commented Jan 29, 2025

We have some logic to don't show the notification once you close it manually at

addons/src/notification.js

Lines 375 to 389 in 3815b3a

closeNotification(e) {
// Avoid going back to the top of the page when closing the notification
e.preventDefault();
// Store the information about dismissal in the Local Storage
// Make sure to store the timestamp, so that we have the option to maybe add a TTL on the dismissal
this.dismissedTimestamp = Date.now();
const dismissedObj = {
[this.localStorageKey]: { dismissedTimestamp: this.dismissedTimestamp },
};
NotificationAddon.setLocalStorage(dismissedObj);
// Avoid event propagation
return false;
}

That logic can be improved to stop showing it in the same domain after 2-3 times being shown, if we want to.

I personally don't find the notification bothering me, but 🤷🏼

@humitos
Copy link
Member Author

humitos commented Jan 29, 2025

However, the logic we should modify here is the triggerAutoDismissTimer to save a counter in the storage. Once that counter reaches 2-3, we can stop showing it. The code lives at

triggerAutoDismissTimer() {
if (!document.hidden && !this.autoDismissed) {
clearTimeout(this.timerID);
this.timerID = setTimeout(() => {
this.autoDismissed = true;
this.requestUpdate();
}, 5000);
}
}

@ericholscher
Copy link
Member

@humitos can we just remove the PR build notification now that we shipped Visual Diff? :D

@agjohnson
Copy link
Contributor

The work we talked about at the offsite will likely supersede this but I'll push it off the current sprint either way.

@humitos
Copy link
Member Author

humitos commented Apr 10, 2025

The work we talked about at the offsite

This is: the notification will be integrated into the UI element displayed in the top center of the page.

@humitos
Copy link
Member Author

humitos commented Apr 29, 2025

I'm going to close this issue since this work will be done in the new UI re-design at #574. Feel free to re-open if you want to work on this issue before the new UI re-design.

@humitos humitos closed this as completed Apr 29, 2025
@github-project-automation github-project-automation bot moved this from Planned to Done in 📍Roadmap Apr 29, 2025
@ericholscher
Copy link
Member

I'd love to just remove the PR build notification right away, if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants