Update notification dropdown styling #2167

bolatovumar · 2020-12-25T03:31:40Z

This styling update brings the notification dropdown styling closer to where it is in the mock-up (https://www.figma.com/file/C7Xyq0FlxgFW8vaBr8ht1z/BTCPAY?node-id=1290%3A379). It's not exactly the same though. Some things that are different:

The notification icon is not the same as in the mock-up. I kept the same icon we currently have. I'm not sure what the approach here should be towards custom icons. I guess I could just plug the SVG in the place of the current icon? Not sure if that's what we want to do.
I didn't make the notification icon green as it ends up looking weird in different themes. I kept the same color we currently have.
The border color is not exactly the same as in the mock-up as we don't have that particular color defined in any of our variables. It is very close to what's in the mock-up though.
The dropdown is not positioned exactly the same as in the mock-up. I left the positioning the same.

Before:

After:

address btcpayserver#2131

bolatovumar · 2020-12-25T03:32:22Z

BTCPayServer/Services/Notifications/Blobs/InvoiceEventNotification.cs

+                {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"}


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

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

Cool, will do.

@Kukks updated here: acbd800

BTCPayServer/Services/Notifications/Blobs/InvoiceEventNotification.cs

bolatovumar · 2020-12-25T03:33:45Z

BTCPayServer/wwwroot/main/bootstrap/bootstrap.css

@@ -6685,6 +6685,9 @@ button.bg-dark:focus {
 .font-weight-normal {
  font-weight: 400 !important; }

+.font-weight-semibold {


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.

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

Cool, thanks!

dennisreimann

Just a few notes. I don't want to delay this as it looks good from a code perspective, but we should note this PR to be revisited with/after the Bootstrap 5 migration (#1866).

I feel we will encounter things like this in a few places from now on until the migration is done. Will schedule the migration for when we are done with the wallet setup refactoring.

dennisreimann · 2020-12-26T15:59:28Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

+<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>
+


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.

@dennisreimann updated here: 2e5b1b4

dennisreimann · 2020-12-26T16:00:53Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

 @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;">


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">

@dennisreimann updated here: 53fa520

dennisreimann · 2020-12-26T16:05:57Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

-        <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>


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.

Ok, I will use a heading.

@dennisreimann updated here: 090c9fc

dennisreimann · 2020-12-26T16:07:05Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

+                        <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>


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.

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

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

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 :)

@dennisreimann alright, added the sprite in e6a3f1e.

Kukks

Agree with @dennisreimann's points but my main concern is the text change for one of the notifications, which should be reverted.

Kukks · 2020-12-27T11:20:31Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

+                        <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>


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

Kukks · 2020-12-27T11:22:19Z

BTCPayServer/Services/Notifications/Blobs/InvoiceEventNotification.cs

+                {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"}


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

bolatovumar · 2020-12-31T02:00:49Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

-                    <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="d-flex mr-3" style="width: 16px; height: 16px; color: #8D8D8F;">


Had to add the inline styles for width and height to make sure the icon appears in the right size since the sprite itself has height and width of zero. I wonder if there should be a built-in way to allow specifying the width and height and fill color? Something like <vc:icon symbol="note" width="16px" height="16px" color="#8D8D8F" />. Or is this not desirable?

There are some icon default styles.

Can we get around using a fixed color here? Because it'll most likely interfere with the different themes. If that color isn't present in the current set of variables, let's use the closest one for now.

@dennisreimann ok, thanks for pointing out the default styles. I will add those in. I initially had the color fixed but changed it to not be fixed as other icons in your PR didn't have them.

@dennisreimann alright, added default styles here which solves the issue nicely. I did notice something weird though. When I updated the color in the icon-sprite.svg my app was refusing to show the new color even after I restarted BTCPay. I had to run dotnet clean and then build from fresh for the change to be reflected. Looked like the SVG was cached and was not updating.

For the color we should stick to using currentColor in the SVG as this allows to set it via CSS. We will need this to work with the different themes.

I also came across caching issues, which I brought up in this comment. I'll try to find a workaround, because this will likely take some time to get fixed.

@dennisreimann alright, updated here: ebdd42e.

I noticed that the cache seems to be cleared if you modify the Default.cshtml under Components/Icon.

dennisreimann · 2021-01-03T14:17:28Z

BTCPayServer/Components/NotificationsDropdown/Default.cshtml

@@ -23,7 +23,7 @@
            @foreach (var notif in Model.Last5)
            {
                <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">
+                    <div class="mr-3" style="color: #8D8D8F;">


We should not go with a fixed color here but use one of the variables to be compatible with the themes. Also I'd rather have this in the site.css that an inline style, because that way it won't get overlooked easily.

@dennisreimann alright, updated here: 4d143d3

dennisreimann · 2021-01-04T23:47:48Z

Nice! I got two minor things I'd propose, because imho …

the headline looks too big (h4), let's make it an h5
the positioning of the dropdown can be improved, especially as it blends too much with the header in the dark theme

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

See proposed screenshots …

Current

Proposed

dstrukt · 2021-01-04T23:54:11Z

Agree with @dennisreimann on the heading element size, the mocks have 20px font-size / 28px line-height, so whatever's the closest equivalent (if that's the H5 in today's system), let's roll with that.
Also agree on the placement of the dropdown.

Otherwise, looks fantastic, great work @bolatovumar @dennisreimann!

bolatovumar · 2021-01-06T01:36:59Z

@dennisreimann adjusted the dropdown positioning here: 5b04ccc

Here is what H5 heading looks like vs the H4 one:

H4

H5

H4 is closer in size to what the mock-up has (mock-up has 20px and H4 is 21px). H5 is 18px. I can update it to be H5 but I picked H4 initially because it was closer in size to what the mock had.

NicolasDorier · 2021-01-06T01:55:11Z

sexy as fuck, let's add this for next release this week.

dstrukt · 2021-01-06T02:20:47Z

@bolatovumar Seeing them both in context, let's roll with H5 (18px), it looks better.

My 2 sats.

Oh, and what do hover states on the notification row look like currently? Only thing I think that's remaining.

bolatovumar · 2021-01-07T00:01:32Z

@bolatovumar Seeing them both in context, let's roll with H5 (18px), it looks better.

My 2 sats.

Oh, and what do hover states on the notification row look like currently? Only thing I think that's remaining.

Updated H4 to H5 here: bcce5e3

Hover state looks like this (see second item):

bolatovumar added 4 commits December 24, 2020 18:44

Update notification dropdown strings

00ae1c7

Update notification dropdown styling

a3c482f

address btcpayserver#2131

Update border color

22ab33e

Update notification hover color

ad90103

bolatovumar commented Dec 25, 2020

View reviewed changes

dennisreimann reviewed Dec 26, 2020

View reviewed changes

Kukks requested changes Dec 27, 2020

View reviewed changes

bolatovumar added 4 commits December 28, 2020 12:28

Revert "was confirmed paid" to "has been paid" change

acbd800

Move styles to site.css

2e5b1b4

Update style="border-bottom: 0;" to border-bottom-0

53fa520

Update heading

090c9fc

bolatovumar requested review from Kukks and dennisreimann December 28, 2020 20:46

Add icon sprite

e6a3f1e

bolatovumar commented Dec 31, 2020

View reviewed changes

bolatovumar added 2 commits January 1, 2021 17:15

Add default icon styles

749efd2

Use "currentColor" instead of specific color for icon

ebdd42e

dennisreimann reviewed Jan 3, 2021

View reviewed changes

Update icon color definition

4d143d3

Adjust dropdown position

5b04ccc

Update h4 to h5

bcce5e3

dennisreimann approved these changes Jan 7, 2021

View reviewed changes

NicolasDorier merged commit fe4ffcf into btcpayserver:master Jan 7, 2021

pavlenex mentioned this pull request Jan 8, 2021

Redesign of the notification's dropdown #2131

Closed

	<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">

Update notification dropdown styling #2167

Update notification dropdown styling #2167

Conversation

bolatovumar commented Dec 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisreimann Dec 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kukks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolatovumar Jan 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisreimann commented Jan 4, 2021 • edited

Current

Proposed

dstrukt commented Jan 4, 2021

bolatovumar commented Jan 6, 2021

NicolasDorier commented Jan 6, 2021

dstrukt commented Jan 6, 2021 • edited

bolatovumar commented Jan 7, 2021 • edited

dennisreimann Dec 29, 2020 •

edited

bolatovumar Jan 2, 2021 •

edited

dennisreimann commented Jan 4, 2021 •

edited

dstrukt commented Jan 6, 2021 •

edited

bolatovumar commented Jan 7, 2021 •

edited