-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
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:
|
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
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.
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. |
Yeah, that makes sense. These options would be farther out anyways, so we can re-evaluate if we ever do want something like this.
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:
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.
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:
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) |
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. |
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. |
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: (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. |
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:
|
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. |
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. |
I agree with this 👍🏼
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. |
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. |
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) |
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. |
Something like this? #283 |
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:
The text was updated successfully, but these errors were encountered: