UI: allow to close notifications #8345
Conversation
stsewd
force-pushed
the
allow-close-notifications
branch
from
bc7c3f6
to
46fbfbc
Compare
| @@ -621,6 +621,14 @@ p.build-missing { font-size: .8em; color: #9d9a55; margin: 0 0 3px; } | |||
| color: #5a5; | |||
| } | |||
|
|
|||
| a.notification-action { | |||
| text-decoration: none; | |||
There was a problem hiding this comment.
This would probably be better floated right. On the left, this feels like more of an icon instead of an action:
readthedocs/templates/base.html
Outdated
| @@ -102,6 +102,11 @@ | |||
| <ul class="notifications"> | |||
| {% for message in messages %} | |||
| <li class="notification notification-{{ message.level }}" {% if message.pk %}data-dismiss-url="{% url 'message_mark_read' message.pk %}{% endif %}"> | |||
| {% if message.pk %} | |||
| <a class="notification-action" href="{% url 'message_mark_read' message.pk %}"> | |||
| <span class="icon close" aria-label="Close notification"></span> | |||
There was a problem hiding this comment.
Alternatively, instead of dropping the icon on both community/commercial, this span could default to text (<span>Close</span>), and on community the span would have text content removed via CSS and replaced with a FA icon
There was a problem hiding this comment.
Also, needs {% trans %} on text
Looks like no, the template is overridden in .com, I'll need to port these changes there :/ |
|
@agjohnson I didn't use float: right because we don't have like a box for the notification, so it looks weird, for .com it looks good.
And about using text, where would you put it? |
|
Also, we need to change this in .com either way, because we don't inherit from this template. What about leaving the icon on the left for .org and make it float 🎈 on .com? |
Yeah true, for most notifications and viewport sizes, it's probably too wide. I'd still put it on the right side of the notification, but perhaps not with floating. The icons will stagger when messages stack up, but that's probably not awful. Commercial notification looks good! |
How would that be? Like at the end of the sentence? That won't work for multi-line notifications |
|
Two options:
|
|
@agjohnson using max-content requires fewer changes. Which one looks better? max-content applied to the whole list or per-item?
|
|
Should be per I'd also increase left padding or margin: |
|
@agjohnson done
|
|
Thanks for jumping on this @stsewd ❤️ |
This is fixed on the new templates too, but we still need more work before releasing them.
I'd put this on the other side, but we don't have something like a box for these notifications