Skip to content
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

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

Open
humitos opened this issue Apr 12, 2023 · 14 comments · May be fixed by #283
Open

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

humitos opened this issue Apr 12, 2023 · 14 comments · May be fixed by #283
Assignees
Labels
Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Apr 12, 2023

It may be useful to save a cookie when the user closes the notification, and use it to decide whether or not show the notification again if the user opens another page.

Overall, I like this feature, but I'm not sure if it may not introduce confusions to the reader:

  • how do we communicate the notification is not going to show it anymore in the next page?
  • would the cookie be deleted once the PR is updated with a new build?
@humitos humitos added the Improvement Minor improvement to code label Apr 12, 2023
@humitos humitos changed the title Notification: use a cookie to keep the notification closed for the session Notification: use a cookie to keep it closed for the session Apr 12, 2023
@agjohnson
Copy link
Contributor

I don't think we should remove any cookies for the user on rebuilds, but we can probably tune the dismissal options to make more sense. A few options:

  • Notification always automatically dismiss after 10-30 seconds, or can be manually dismissed. We treat manual dismissal as a stronger signal that the user doesn't want future notifications.
  • The close button is actually a dropdown or a button + dropdown. The button dismisses the notification temporarily, the dropdown menu has menu options "Dismiss for now" and "Permanently dismiss".
    • Perhaps we even give a "Permanently dismiss for future pull requests" option

@humitos
Copy link
Member Author

humitos commented Sep 5, 2023

I think it would be good to have a way to dismiss the notification on the current version 👍🏼 . However, I'd like to keep the implementation simple for us and clear for the user. My comments below

Notification always automatically dismiss after 10-30 seconds, or can be manually dismissed. We treat manual dismissal as a stronger signal that the user doesn't want future notifications.

Personally, I don't like notifications that disappears for multiple reasons. Sometimes, it's shown while I don't have the tab open and I'm not able to see it even. In other cases, I'm the time defined to dismiss is not enough to me to be able to read the complete text. Besides, even if I'm able to read it completely, there are some notifications that have links that I want to be able to click after the dismiss time.

The close button is actually a dropdown or a button + dropdown. The button dismisses the notification temporarily, the dropdown menu has menu options "Dismiss for now" and "Permanently dismiss". Perhaps we even give a "Permanently dismiss for future pull requests" option

Any option that means "Permanently dismiss" is kind of a problem to me since it requires a more complex UX and also it also requires to build the ability for the user to re-enable them. I'm imaging a lot of users clicking it by mistake and not knowing how to re-enable it, too.


Ideally, to me there should be a way to "disable this exact notification but keep showing it when I load another page from the same documentation version" (current behavior) and/or "disable this exact notification and don't show it again for this particular version" (I already know that I'm browsing a PR) --on a different PR, the notification will appear again.

Note that I don't want o make this a per-user setting because it will make current endpoint more complex and harder to cache. I suppose that eventually we may want to have per-user settings, but I think we could use a different endpoint for that that won't be cached. However, I think we are not yet there and a "domain & path including the version" cookie-based approach should be enough.

@agjohnson
Copy link
Contributor

Note that I don't want o make this a per-user setting because it will make current endpoint more complex

Yeah, that makes sense. These options would be farther out anyways, so we can re-evaluate if we ever do want something like this.

Any option that means "Permanently dismiss" is kind of a problem to me since it requires a more complex UX

The options can be tweaked, but the UX I'm describing is still relevant. I'd use a dropdown to give users multiple closing options in a single button:

image

"disable this exact notification but keep showing it when I load another page from the same documentation version"

I agree on keeping this as an option, but I'd rather this be the secondary UX, not the primary. If the user was bothered by this notification enough to close it, they probably are going to be just as bothered by it on the next page load too.

Especially in the case of PR build notifications, I've found per-page notifications quite disruptive.

Personally, I don't like notifications that disappears for multiple reasons.

I still think adding a timed option to the notification is the better UX overall, as it both offers the warning without requiring interaction on every page load. The issues you note with timed notifications have some simple solutions:

  • Using the page visibility API allows us to dismiss the notification only if the tab is in view.
  • Pausing the dismiss timer on mouse hover allows for reading of the text and clicking links.
  • Visual feedback on the timer helps readability too

However, if we are only implementing one option right now, I think it should be explicit buttons. I definitely would not rule timed notifications out though. It's a great way to get the notification on every page without annoying the reader (as much)

@humitos
Copy link
Member Author

humitos commented Sep 5, 2023

All you've said here is great! 💯 Take all my money 💵 -- 😉 . Now, I don't have a strict preference on either. It seems that if the auto-dismiss is well implemented it could work fine to me as well.

@agjohnson
Copy link
Contributor

I'd say that if we want explicit dismiss options in either case, let's start there and save the auto-dismiss version for later. I think the auto-dismiss version will be nice, polished UX, but it will take a bit more UI work to get it right.

@zanderle
Copy link
Collaborator

zanderle commented Mar 31, 2024

An alternative suggestion on the auto-dismiss: instead of completely dismissing the notification, what if (after a certain timeout, along with the solutions proposed above) we simply make the notification less visible. Something like this:

Screenshot 2024-04-01 at 00 14 17

(the close button could also just show up on hover, to make it even smaller)

That way we keep the reminder that this is from a PR build, but we also make it less intrusive, since the user will know what it means, so they don't need the full text all the time.

@agjohnson
Copy link
Contributor

This could definitely be an option as well. The next question for me might be whether we just want to roll the PR build warning into the flyout display (primarily the unexpanded view). This is maybe getting more into the conversation around a newly designed flyout rather than changing the current flyout.

I say we should start considering these options for the next step of this UI/UX. It seems we have some good ideas we need to weigh.

To me, the order of work here feels like:

  • Implement cookie/local storage tracking of per-build notification dismissal. Subsequent page loads don't show the notification at all.
  • Next, weigh if we want an option to dismiss for the reader globally, across all builds/version.
  • Then finally, polish the UX/UI with some of the options we've discussed. I think we can use this step to both make the notifications less intrusive for more readers and also show these elements to users that may have dismissed the notifications previously -- timed notifications, collapsing elements, or even flyout color/icon/additional elements all feel like possible options.

@zanderle
Copy link
Collaborator

zanderle commented Apr 3, 2024

Next, weigh if we want an option to dismiss for the reader globally, across all builds/version.

Personally, I don't see why that needs to be an option. To me, this notification seems like something that needs to be present to make sure it's always clear that this is a PR build. Sure, let's make it less intrusive, but I think having it present is a nice (and actually a very common - I doubt it's too annoying to anyone) feature.

@zanderle
Copy link
Collaborator

zanderle commented Apr 3, 2024

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

Another consideration I wanted to bring up: do we want to somehow isolate these dismissals? In other words, do we need to account for multiple notifications on a single page - should they be sharing the dismissals or not? If so, we should maybe make the site worry about storing the dismissal, and the web component would simply trigger an event when close button is pressed and accept an attribute, when it's initialized (to determine whether it should be shown or not). This would be a bit more work, but it would be a bit more aligned with the idea of web components being isolated/encapsulated.

@humitos
Copy link
Member Author

humitos commented Apr 3, 2024

Personally, I don't see why that needs to be an option. To me, this notification seems like something that needs to be present to make sure it's always clear that this is a PR build. Sure, let's make it less intrusive, but I think having it present is a nice

I agree with this 👍🏼

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

Another consideration I wanted to bring up: do we want to somehow isolate these dismissals? In other words, do we need to account for multiple notifications on a single page - should they be sharing the dismissals or not?

We could eventually have multiple notifications on a single page, yes. I think it's fine to track each notification separately here and don't show the notification if it was dismissed for the whole build.

@agjohnson
Copy link
Contributor

agjohnson commented Apr 3, 2024

Personally, I don't see why that needs to be an option. To me, this notification seems like something that needs to be present to make sure it's always clear that this is a PR build.

To clarify, I'm not talking about making this a user configurable option. I just meant a collapsible notification is a potential direction we can take. I agree the PR build notification information is useful either way.

But if these notifications (pr build version, latest version, etc) collapse down to floating icons, that also feels like state we could just represent on our flyout too, as opposed to having multiple, small, floating elements on the user's page.

That is, the notification can be dismissed, but we use the flyout to always show pr build/latest version/etc state using an always visible element/icon/something.

I would take a collapsing notification over what we have now, but would love to see the flyout have more useful features too.

@humitos
Copy link
Member Author

humitos commented Apr 4, 2024

I would take a collapsing notification over what we have now, but would love to see the flyout have more useful features too.

Gotcha. I understand. I prefer to fix these issues (add features) without touching the flyout layout yet and plan a full redesign of it for a v2 in the future (see #53)

@agjohnson
Copy link
Contributor

Yup, that is what I mentioned above too: #46 (comment)

Adding full dismissal functionality is the first step in any direction. I don't feel we should be working yet on add new display feature to notifications -- collapsing, timeout, or moving to the flyout.

@zanderle zanderle linked a pull request Apr 5, 2024 that will close this issue
@zanderle
Copy link
Collaborator

zanderle commented Apr 5, 2024

Something like this? #283

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
Status: Needs review
Development

Successfully merging a pull request may close this issue.

3 participants