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.

Sign up for GitHub

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

Jump to bottom

Update notification dropdown styling #2167

Merged
merged 14 commits into from
Jan 7, 2021
Merged

Update notification dropdown styling #2167

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
00ae1c7
Update notification dropdown strings
bolatovumar Dec 25, 2020
a3c482f
Update notification dropdown styling
bolatovumar Dec 25, 2020
22ab33e
Update border color
bolatovumar Dec 25, 2020
ad90103
Update notification hover color
bolatovumar Dec 25, 2020
acbd800
Revert "was confirmed paid" to "has been paid" change
bolatovumar Dec 28, 2020
2e5b1b4
Move styles to site.css
bolatovumar Dec 28, 2020
53fa520
Update style="border-bottom: 0;" to border-bottom-0
bolatovumar Dec 28, 2020
090c9fc
Update heading
bolatovumar Dec 28, 2020
e6a3f1e
Add icon sprite
bolatovumar Dec 31, 2020
749efd2
Add default icon styles
bolatovumar Jan 2, 2021
ebdd42e
Use "currentColor" instead of specific color for icon
bolatovumar Jan 3, 2021
4d143d3
Update icon color definition
bolatovumar Jan 3, 2021
5b04ccc
Adjust dropdown position
bolatovumar Jan 6, 2021
bcce5e3
Update h4 to h5
bolatovumar Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 44 additions & 11 deletions BTCPayServer/Components/NotificationsDropdown/Default.cshtml
View file
Open in desktop
Expand Up @@ -5,29 +5,62 @@
@using Microsoft.AspNetCore.Http.Extensions
@model BTCPayServer.Components.NotificationsDropdown.NotificationSummaryViewModel

<style>
.notification-dropdown {
border: 0;
border-radius: 4px;
box-shadow: 0px 2px 16px rgba(0, 0, 0, 0.08);
padding: 0;
}

@@media (min-width: 992px) {
.notification-dropdown {
width: 420px;
}
}

.notification:hover {
background-color: var(--btcpay-body-bg);
}
</style>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this to the site.css – this way it can be cached and it might not be as easy to overlook there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisreimann updated here: 2e5b1b4

@if (Model.UnseenCount > 0)
{
<li class="nav-item dropdown" id="notifications-nav-item">
<a class="nav-link js-scroll-trigger" href="#" id="navbarDropdown" role="button" data-toggle="dropdown" id="Notifications">
<a class="nav-link js-scroll-trigger" href="#" id="navbarDropdown" role="button" data-toggle="dropdown" id="Notifications" style="border-bottom: 0;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a class="nav-link js-scroll-trigger" href="#" id="navbarDropdown" role="button" data-toggle="dropdown" id="Notifications" style="border-bottom: 0;">
<a class="nav-link js-scroll-trigger border-bottom-0" href="#" id="navbarDropdown" role="button" data-toggle="dropdown" id="Notifications">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisreimann updated here: 53fa520

<i class="fa fa-bell"></i>
</a>
<span class="alerts-badge badge badge-pill badge-danger">@Model.UnseenCount</span>
<div class="dropdown-menu dropdown-menu-right text-center notification-items" aria-labelledby="navbarDropdown">
<div class="dropdown-menu dropdown-menu-right text-center notification-dropdown" aria-labelledby="navbarDropdown">
<div class="d-flex align-items-center justify-content-between py-3 px-4 border-bottom border-light">
<div role="heading" aria-level="3" class="font-weight-semibold" style="font-size: 20px;">Notifications</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a heading here – even if it doesn't closely match the mockup with size and weight. My point would be that we should be consistent with the tools we have available currently (Bootstrap 4.5) and do the cleanup/extension in one go. This isn't ideal either, but I fear we will miss things if we are using inline styles and one-off solutions like this one, without marking them for the upcoming UI refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will use a heading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisreimann updated here: 090c9fc

<form asp-controller="Notifications" asp-action="MarkAllAsSeen" asp-route-returnUrl="@Context.Request.GetCurrentPathWithQueryString()" method="post">
<button class="btn btn-link p-0 font-weight-semibold" type="submit">Mark all as seen</button>
</form>
</div>
@foreach (var notif in Model.Last5)
{
<a asp-action="NotificationPassThrough" asp-controller="Notifications" asp-route-id="@notif.Id" class="dropdown-item border-bottom py-2 px-3">
<div class="text-left" style="width: 200px; white-space:normal;">
@notif.Body
<a asp-action="NotificationPassThrough" asp-controller="Notifications" asp-route-id="@notif.Id" class="notification d-flex align-items-center dropdown-item border-bottom border-light py-3 px-4">
<div class="mr-3">
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M14.2 16H1.8C0.808 16 0 15.192 0 14.2V1.8C0 0.808 0.808 0 1.8 0H14.2C15.192 0 16 0.808 16 1.8V14.2C16 15.192 15.192 16 14.2 16ZM1.8 1.2C1.4688 1.2 1.2 1.4696 1.2 1.8V14.2C1.2 14.5304 1.4688 14.8 1.8 14.8H14.2C14.5312 14.8 14.8 14.5304 14.8 14.2V1.8C14.8 1.4696 14.5312 1.2 14.2 1.2H1.8Z" fill="#8D8D8F"/>
<path d="M12.0004 5.31182H4.00039C3.66919 5.31182 3.40039 5.04222 3.40039 4.71182C3.40039 4.38142 3.66919 4.11182 4.00039 4.11182H12.0004C12.3316 4.11182 12.6004 4.37982 12.6004 4.71182C12.6004 5.04382 12.3316 5.31182 12.0004 5.31182ZM12.0004 8.59982H4.00039C3.66919 8.59982 3.40039 8.33102 3.40039 7.99982C3.40039 7.66862 3.66919 7.39982 4.00039 7.39982H12.0004C12.3316 7.39982 12.6004 7.66862 12.6004 7.99982C12.6004 8.33102 12.3316 8.59982 12.0004 8.59982ZM8.00039 11.8878H4.00039C3.66919 11.8878 3.40039 11.6198 3.40039 11.2878C3.40039 10.9558 3.66919 10.6878 4.00039 10.6878H8.00039C8.33159 10.6878 8.60039 10.9574 8.60039 11.2878C8.60039 11.6182 8.33159 11.8878 8.00039 11.8878Z" fill="#8D8D8F"/>
</svg>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the wallet setup UI refactoring I've introduced a SVG icon sprite for our custom icons. We can add this there once the other PR is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still move this to a separate svg file in this PR to avoid rendering bigger HTML markup since this is a list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisreimann ok, I will update this once the other PR is merged. Could you link it here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's #2164, but it'll likely take some time to get merged. Let's not wait for this, because it would postpone this otherwise ready PR.

Instead, you can add the icon sprite file with the desired icons here too. There's also the Icon view component to use the icons in that sprite.
You can also give me access to your fork and I can add all of that to this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisreimann alright, added the sprite in e6a3f1e.

</div>
<div class="text-left">
<small class="text-muted" data-timeago-unixms="@notif.Created.ToUnixTimeMilliseconds()">@notif.Created.ToTimeAgo()</small>

<div class="notification-item__content">
<div class="text-left font-weight-semibold">
@notif.Body
</div>
<div class="text-left d-flex">
<small class="text-muted" data-timeago-unixms="@notif.Created.ToUnixTimeMilliseconds()">@notif.Created.ToTimeAgo()</small>
</div>
</div>
</a>
}
<a class="dropdown-item text-secondary" asp-controller="Notifications" asp-action="Index">See All</a>
<form asp-controller="Notifications" asp-action="MarkAllAsSeen" asp-route-returnUrl="@Context.Request.GetCurrentPathWithQueryString()" method="post">
<button class="dropdown-item text-secondary" type="submit"><i class="fa fa-eye"></i> Mark all as seen</button>
</form>
<div class="p-3">
<a class="font-weight-semibold" asp-controller="Notifications" asp-action="Index">View all</a>
</div>
</div>
</li>
}
Expand Down
12 changes: 6 additions & 6 deletions BTCPayServer/Services/Notifications/Blobs/InvoiceEventNotification.cs
View file
Open in desktop
Expand Up @@ -36,12 +36,12 @@ public Handler(LinkGenerator linkGenerator, BTCPayServerOptions options)

internal static Dictionary<string, string> TextMapping = new Dictionary<string, string>()
{
// {InvoiceEvent.PaidInFull, "was fully paid."},
{InvoiceEvent.PaidAfterExpiration, "was paid after expiration."},
{InvoiceEvent.ExpiredPaidPartial, "expired with partial payments."},
{InvoiceEvent.FailedToConfirm, "has payments that failed to confirm on time."},
// {InvoiceEvent.ReceivedPayment, "received a payment."},
{InvoiceEvent.Confirmed, "was confirmed paid."}
// {InvoiceEvent.PaidInFull, "was fully paid"},
{InvoiceEvent.PaidAfterExpiration, "was paid after expiration"},
Kukks marked this conversation as resolved.
Show resolved Hide resolved
{InvoiceEvent.ExpiredPaidPartial, "expired with partial payments"},
{InvoiceEvent.FailedToConfirm, "has payments that failed to confirm on time"},
// {InvoiceEvent.ReceivedPayment, "received a payment"},
{InvoiceEvent.Confirmed, "has been paid"}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this to match the wording in the mock-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert. This makes the notification unclear whether the invoice was paid and settled or just paid and unconfirmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kukks updated here: acbd800

};

protected override void FillViewModel(InvoiceEventNotification notification,
Expand Down
3 changes: 3 additions & 0 deletions BTCPayServer/wwwroot/main/bootstrap/bootstrap.css
View file
Open in desktop
Expand Up @@ -6685,6 +6685,9 @@ button.bg-dark:focus {
.font-weight-normal {
font-weight: 400 !important; }

.font-weight-semibold {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this here because the mock-up specified semibold weights but we didn't have a utility class for it like we have for other font weights.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted as a thing to add/update with our Bootstrap 5 migration (#1866).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

font-weight: 600 !important; }

.font-weight-bold {
font-weight: 700 !important; }

Expand Down