Make notifications permanently dismissed #283

zanderle · 2024-04-05T16:37:22Z

POC for now

Closes #46

to make it easier for future changes

humitos

I think this is going in the direction we need. However, there are some changes in the logic we should make here.

From @agjohnson's comment in #46 (comment)

Implement cookie/local storage tracking of per-build notification dismissal. Subsequent page loads don't show the notification at all.

I understand we shouldn't dismiss this notification for all the PRs just by closing the notification, but only on this particular build. So, we should save the build.pk in the storage so we know on which build we shouldn't show.

That is, closing the notification on the first page, won't show it on any of the following page from the same build. Then, if the PR is rebuilt, it will show the notification again.

Besides, if the user opens a different PR preview the notification will be shown again.

Also, note that this addons shows a notification for PR builder and for stable/latest versions so the storage will affect those as well. We may want to split this notification addon (see #201) before moving forward with this or consider this scenario here.

humitos · 2024-04-08T08:54:39Z

src/notification.js

+  getLocalStorage() {
+    const notificationStorage = window.localStorage.getItem(this.notificationAddonLocalStorageKey);
+    return notificationStorage ? JSON.parse(notificationStorage) : {};
+  }
+
+  setLocalStorage(obj) {
+    const notificationStorage = this.getLocalStorage() || {};
+    const specificStorage = notificationStorage[this.localStorageKey] || {};
+    notificationStorage[this.localStorageKey] = {...specificStorage, ...obj};
+    window.localStorage.setItem(this.notificationAddonLocalStorageKey, JSON.stringify(notificationStorage));
+  }
+


I'd like to standardize the usage of the storage now that we are going to use it in more than one addons.

These two functions should probably be moved to src/utils.js/AddonsBase and re-use them from src/search.js too since we are saving the recent searches there.

I think each addon should save one object on the storage instead of each separate value at the top level.

this.storageKey = "readthedocs-${this.addonName.lower()}" // ... window.localStorage.setItem( this.storageKey, { dismissed: true, }, );

Since we need these methods inside actual web-components, does it make sense to define these methods in AddonBase? We won't be using it in the addon classes, but inside NotificationElement, SearchElement, and so on.
Or were you thinking of simply calling NotificationAddon.setLocalStorage(obj) from within NotificationElement? That works, just wanted to confirm first.

I don't have too much experience with Js inheritance, so maybe @agjohnson is the one here to ask.

We could either:

create a ReadTheDocsElement and make all the Element classes inherit from it

call these methods from Addon.* as you mentioned

I don't have a strong preference here. My original goal was to share this code among addons/elements since it will be exactly the same for all of them.

I'll figure something out. I just thought you had a specific solution in mind so I wanted to make sure I'm not missing something :)

Either option sounds good, I might lean towards using the AddonBase class though, as we are loading our configuration at the addon class level already. It's a fuzzy sort of split between the two though.

I think the only reason against using the AddonBase is that it makes it a bit more messy, when we want to do storage per component (versus per addon). If we'll have that requirement only for Notifications, then I guess it's not such a big deal, but if we'll have it elsewhere, then I think we need to put it on the Element. WDYT? @humitos @agjohnson

That's a good point. I don't have any strong opinions here either, this is an internal method too so we can always change our minds and move code around later. Whatever feels the most intuitive here 👍

src/notification.js

…cations

humitos

This approach looks good to me! 👍🏼

Is it ready to merge? I'm asking because it's in Draft and I'm not sure if you are still working on it or not, @zanderle

zanderle · 2024-05-15T08:27:55Z

This approach looks good to me! 👍🏼

Is it ready to merge? I'm asking because it's in Draft and I'm not sure if you are still working on it or not, @zanderle

No, I just need to use these new functions in the search addon as well.

Also are we ok with the behaviour implemented here (in terms of permanently dismissing the notification), or does that require further testing/discussion?

humitos · 2024-05-22T09:36:54Z

I think this approach is fine. I understand the local storage is based on the domain, right? In that case, dismissing a notification on a pull request won't affect latest/stable notifications, which is great.

However, dismissing a latest notification on https://docs.readthedocs.io/en/latest/ will also dismiss the stable notification on the same domain, for example when hitting https://docs.readthedocs.io/en/0.9.x/. So, I suppose the local storage key should be based on the version as well: with a postfix of this.config.versions.current.slug probably to make it explicit.

Am I correct?

humitos · 2024-05-27T15:14:03Z

We also serve multiple projects under the same domain, when they are subprojects. So, I think we should save the project-slug and version-slug in the local storage key as well.

agjohnson · 2024-05-28T17:06:06Z

Tracking this per subproject could be too granular, and tracking per version feels really granular. There is a good chance that if a user dismisses this once they will want it dismissed everywhere. If a user has to dismiss notifications for each version, it will feel like dismissal isn't working correctly.

For me, this keeps coming back to prompting the user for what they want -- dismiss for now, for this project, for all projects of parent project, etc.

For that, tracking the subproject is okay, as long as our plan for storage/data allow these patterns later.

humitos · 2024-05-29T10:03:04Z

Tracking this per subproject could be too granular, and tracking per version feels really granular. There is a good chance that if a user dismisses this once they will want it dismissed everywhere.

If the user see a notification "this is an old version" for a project and dismiss it, and then open an old version of a subproject, I think we should show the notification again. They are different projects and different versions, so the user should be warned about reading an outdated version of the subproject to avoid confusions.

That's the v1 of the implementation. In a next iteration we can work on the granularity and configuration/prompting, but we are not there yet.

agjohnson · 2024-05-29T16:36:07Z

That is fine for the latest/stable build warning, but the PR build warning is still fairly different.

As a user, I understand what I'm seeing is a PR build after clicking through from the PR once or twice. Reminding me every time, and requiring me to close the notification every PR, is what feels redundant. This notification is in a different class as it's not for reader users but for the maintainer, who interacts with the notification frequently.

humitos · 2024-05-30T10:07:55Z

Yes, PR notification and stable/latest should behave differently. In fact, I want to split this addon into different ones because we've reach this differences in behavior multiple times already and having only 1 addon for all of them makes everything harder and more confusing.

humitos · 2024-05-30T10:12:28Z

@zanderle can you implement the initial version of the dismiss that consider domain, project, version and language slug?

Behavior:

dismissing the notification won't show it again on the same project, version, language
switching any of them will make the notification to show again
dismissing the notification on PR builds, won't show it again on that PR preview

In the next iteration we will split the notification addon and consider the user configuration (e.g. dismiss all PR notifications always for all the domains, etc)

zanderle · 2024-05-30T12:03:05Z

@humitos I think I can. Let me dig in, and I'll let you know in case I run into any complications.

zanderle added 2 commits April 3, 2024 14:07

Extract the notification close button into a function

ef73467

to make it easier for future changes

Make notifications permanently dismissed using localStorage

02d8dc6

zanderle requested review from humitos and a team as code owners April 5, 2024 16:37

zanderle requested a review from agjohnson April 5, 2024 16:37

zanderle mentioned this pull request Apr 5, 2024

Notification: use a cookie to keep it closed for the session #46

Open

humitos requested changes Apr 8, 2024

View reviewed changes

Make dismissed a timestamp rather than a boolean

311808d

humitos marked this pull request as draft April 16, 2024 08:34

zanderle added 4 commits April 25, 2024 18:25

Implement methods to deal with local storage on AddonBase

adc716a

Refactor notifications to use AddonBase local storage methods

ea35431

Merge remote-tracking branch 'origin/main' into zanderle/close-notifi…

845c62d

…cations

Add more comments

3a649cf

zanderle requested a review from humitos April 25, 2024 17:14

humitos approved these changes May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make notifications permanently dismissed #283

Make notifications permanently dismissed #283

zanderle commented Apr 5, 2024

humitos left a comment

humitos Apr 8, 2024

zanderle Apr 12, 2024

humitos Apr 15, 2024

zanderle Apr 15, 2024

agjohnson Apr 15, 2024

zanderle Apr 17, 2024

agjohnson Apr 17, 2024

humitos left a comment

zanderle commented May 15, 2024

humitos commented May 22, 2024

humitos commented May 27, 2024

agjohnson commented May 28, 2024 •

edited

humitos commented May 29, 2024 •

edited

agjohnson commented May 29, 2024

humitos commented May 30, 2024

humitos commented May 30, 2024

zanderle commented May 30, 2024

Make notifications permanently dismissed #283

Are you sure you want to change the base?

Make notifications permanently dismissed #283

Conversation

zanderle commented Apr 5, 2024

humitos left a comment

Choose a reason for hiding this comment

humitos Apr 8, 2024

Choose a reason for hiding this comment

zanderle Apr 12, 2024

Choose a reason for hiding this comment

humitos Apr 15, 2024

Choose a reason for hiding this comment

zanderle Apr 15, 2024

Choose a reason for hiding this comment

agjohnson Apr 15, 2024

Choose a reason for hiding this comment

zanderle Apr 17, 2024

Choose a reason for hiding this comment

agjohnson Apr 17, 2024

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

zanderle commented May 15, 2024

humitos commented May 22, 2024

humitos commented May 27, 2024

agjohnson commented May 28, 2024 • edited

humitos commented May 29, 2024 • edited

agjohnson commented May 29, 2024

humitos commented May 30, 2024

humitos commented May 30, 2024

zanderle commented May 30, 2024

agjohnson commented May 28, 2024 •

edited

humitos commented May 29, 2024 •

edited