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

Notificationlist #6050

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Notificationlist #6050

wants to merge 12 commits into from
+230 −59

Conversation

TChukwuleta
Copy link
Contributor

Resolves #3871

@dstrukt did Include the store column, as not all notifications are associated with a store.

But outside of that, open to review.

dstrukt
dstrukt suggested changes Jun 12, 2024
View reviewed changes
Copy link
Member

@dstrukt dstrukt left a comment •
edited

Choose a reason for hiding this comment

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

Overall, this looks great! Awesome work!

General

  • let's kill the ms-3 class on all of the buttons
  • we could increase the gap-3 to gap-4 at the parent form level

these spacing changes could help solve some of the problems below

  • For the types, let's combine the User related ones to User Updates
  • combine Payout+External Payout -> Payouts
  • New Version to BTCPay Server Updates or Server Updates
  • Plugin Update to Plugin Updates

Mobile

  • Ideally the tabs sit flush to the left
  • Also, it would be nice if everything sat on the same line
Screen Shot 2024-06-12 at 4 53 45 PM
  • Some things get cut off on the tablet breakpoint, not sure what we can do about this?
Screen Shot 2024-06-12 at 4 54 09 PM
  • there's an in-between state between tablet and mobile where the options are hidden, minor, but worth noting
Screen Shot 2024-06-12 at 5 46 01 PM

Otherwise, I think we're good to go!

@dennisreimann It'd be great to get another pair of eyes on this, and @pavlenex it'd be great to get a functional review from you!

@dstrukt dstrukt requested a review from pavlenex June 12, 2024 23:52
@dennisreimann
Copy link
Member

dennisreimann commented Jun 13, 2024 •
edited

Looking good! Some notes of things I came across:

  • The All/Unread switch could have the primary class for highlighting instead of secondary. There's no CTA on that page and imho that toggle has the most important functionality.
  • Use the gap-X classes instead of ms-X and me-X when working with spacings inside flex containers where possible. It has the benefit of working with flex-wrap automatically and not having to adjust start and end margins on a breakpoint bases.
  • For the wallet settings we use the settings icon (cogwheel) in the button — let's do that here too.
  • When toggling the mass actions, the table bounced. We had that on the other views too and it was do to the switch time format button line-height. Fixed it.
  • So that we don't forget: We'll need to adapt the header here as well, once UI: Move section navigation to sidebar #5744 is merged.

I think once dstrukt's feedback is incorporated we can have a final round of review and merge it. Good work!

@TChukwuleta
Copy link
Contributor Author

Alright. Will update later accordingly

Thank you

dennisreimann
dennisreimann reviewed Jun 13, 2024
View reviewed changes
Copy link
Member

@dennisreimann dennisreimann left a comment

Choose a reason for hiding this comment

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

Code LGTM so far. The addition of the store ID to the notifications will be very helpful for the app as well.

BTCPayServer/Controllers/UINotificationsController.cs Outdated Show resolved Hide resolved
@dstrukt
Copy link
Member

dstrukt commented Jun 13, 2024

  • The All/Unread switch could have the primary class for highlighting instead of secondary. There's no CTA on that page and imho that toggle has the most important functionality.

Completely agree, and was going to do a follow up design component issue, but if we can tackle here, that'd be great!

@TChukwuleta
Copy link
Contributor Author

made the update

dstrukt
dstrukt suggested changes Jun 14, 2024
View reviewed changes
Copy link
Member

@dstrukt dstrukt left a comment •
edited by dennisreimann

Choose a reason for hiding this comment

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

Thanks for the changes, it looks excellent! All of my requested changes look good - last 2 nits and we're good! Great work!

  • let's make the active tab our primary green / variable, not the darker green
Screen Shot 2024-06-14 at 4 38 37 PM
  • can we kill the gap-4 class on mobile breakpoint to tighten up the height?
Screen Shot 2024-06-14 at 4 39 50 PM

Otherwise, this looks good to go!

Requested re-review from @dennisreimann in case he has anything else.

@dstrukt
Copy link
Member

dstrukt commented Jun 15, 2024

Tested the latest, DM'd a quick fix, will approve after!

@dstrukt dstrukt self-requested a review June 15, 2024 14:11
dstrukt
dstrukt approved these changes Jun 15, 2024
View reviewed changes
Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

tACK - LGTM

dennisreimann
dennisreimann approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@dennisreimann dennisreimann left a comment

Choose a reason for hiding this comment

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

tACK, good job!

@dstrukt
Copy link
Member

dstrukt commented Jun 19, 2024 •
edited

Was this a new/additional review request @pavlenex? Or due to potential merge conflicts from the sidebar PR (#5744)?

@dennisreimann
Copy link
Member

Will rebase this and update with the changes from the sidebar/subnav PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstrukt dstrukt dstrukt approved these changes

@pavlenex pavlenex Awaiting requested review from pavlenex

@dennisreimann dennisreimann Awaiting requested review from dennisreimann

Assignees
No one assigned
Labels
None yet
Projects
BTCPay Server Roadmap 🛣️
Status: Backlog 🪵
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improved Notifications List View
3 participants
@TChukwuleta @dennisreimann @dstrukt