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.

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

Add wallet limit banner #699

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Dec 20, 2023

https://stacker.news/items/356071

2023-12-20.06-14-02.mp4

2023-12-20-061636_1920x1080_scrot

2023-12-20-061622_1920x1080_scrot

ignore actual wallet balance in screenshots, I manually set limitReached to true

So basically, this adds a banner that only shows up if the wallet balance is over the limit. It can be dismissed on the front page but it will always appear in the wallet (if over the limit).

@ekzyis ekzyis marked this pull request as draft December 20, 2023 05:08
@ekzyis ekzyis force-pushed the wallet-limit-banner branch from 62a647f to 0aec860 Compare December 20, 2023 05:13
@ekzyis ekzyis marked this pull request as ready for review December 20, 2023 05:15
@ekzyis ekzyis added the feature new product features that weren't there before label Dec 20, 2023
@@ -65,3 +67,50 @@ export default function WelcomeBanner () {
</Alert>
)
}

export function WalletLimitBanner ({ dismissible }) {
// TODO refactor this since it's mostly the same as the code for the welcome banner
Copy link
Member Author

@ekzyis ekzyis Dec 20, 2023

Choose a reason for hiding this comment

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

Did not want to mix feature + refactor in one PR

@kepford
Copy link

kepford commented Dec 20, 2023

No familiar with the code base but this MR looks good to me. Nice demo video as well @ekzyis!

@huumn
Copy link
Member

huumn commented Dec 21, 2023

ACK the concept but I'm not sure the banner should be on the homepage. It should definitely be on the wallet pages and maybe show an indicator on the wallet ... banners on the homepage always feel like an assault.

@ekzyis
Copy link
Member Author

ekzyis commented Dec 21, 2023

banners on the homepage always feel like an assault.

I agree that they are an assault but is this still a problem if you can dismiss them like the welcome banner? I see it as just a way to tell everyone about important things (like SNL being on air)

I am open to remove it of course, just wanted to clarify that this isn't a banner that always shows up but only once until dismissed and also only if the wallet is over the limit.

maybe show an indicator on the wallet

but I also like this idea. But should this indicator then always be there while they are over the limit? Or also just once until clicked? I think both approaches could be confusing since:
a) the indicator is always there: why is it still there after i clicked on my wallet?
b) the indicator disappears but the reason why the indicator was there might still exist (wallet over the limit): why is it no longer there?

Maybe we could create a notification if someone goes over the limit? Then they would only need to check their notifications and it's a consistent UX regarding the indicator behavior.

@ekzyis ekzyis marked this pull request as draft December 21, 2023 10:47
@huumn
Copy link
Member

huumn commented Dec 21, 2023

Maybe it can work like this:

  1. no banner on homepage
  2. when they go over the limit, turn their sats in the upper right hand corner orange
  3. non-dismissable banner on the wallet page

@ekzyis ekzyis force-pushed the wallet-limit-banner branch from 4672396 to d01164f Compare December 21, 2023 21:25
@ekzyis
Copy link
Member Author

ekzyis commented Dec 21, 2023

Maybe it can work like this:

  1. no banner on homepage
  2. when they go over the limit, turn their sats in the upper right hand corner orange
  3. non-dismissable banner on the wallet page

Did that in d01164f now:

2023-12-21.22-24-23.mp4

@ekzyis ekzyis marked this pull request as ready for review December 21, 2023 21:29
@huumn huumn merged commit 01984c0 into stackernews:master Dec 21, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants