-
Notifications
You must be signed in to change notification settings - Fork 16
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 notification when storage is running low #2148
Conversation
Your Render PR Server URL is https://chainsafe-components-stage-pr-2148.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca5qrvqrrk0flptcs1vg. |
Your Render PR Server URL is https://files-ui-stage-pr-2148.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca5qs0qrrk0flptcs240. |
Your Render PR Server URL is https://storage-ui-stage-pr-2148.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca5qs1qrrk0flptcs2lg. |
const [unpaidInvoiceNotification, setUnpaidInvoiceNotification] = useState<string | undefined>() | ||
const [cardExpiringNotification, setCardExpiringNotification] = useState<string | undefined>() | ||
const [isBillingEnabled, setIsBillingEnabled] = useState(false) | ||
const shouldProposeUpgrade = useMemo(() => storageSummary | ||
? storageSummary.used_storage > storageSummary.total_storage * 0.75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asnaith or @juans-chainsafe if you want to test, just change this line to whatever percentage you want, to see the notification appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done! and yep I tested locally because I needed to change the percentage, is working correctly! based on Andrew ticket.
PS: I will do the translations with Weblate to see how it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, leaving a comment to check isBillingEnable
before showing notification.
Thanks for the tips on how to test! This is working well for most parts, just noticed the same thing as Tanmoy - users not whitelisted for billing can still click the notification but as they don't have access to billing it takes them to an empty settings page. I think we should just tell the user their storage is low and remove the "upgrade here" text / click through if they're not whitelisted. |
I added a proper redirection to our form, and as a side note, I asked the design team to change the copy a bit to give more context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go !
Nice change, looks good to go now :) |
closes #2146
I added the ability to close "turn off" a notification on click, until the next refresh at least :)