-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Usage-based] Update billing page layout for teams and personal accounts #13564
Conversation
/werft run 👍 started the job as gitpod-build-jx-improve-billing-ui.1 |
6eedc93
to
4cfb758
Compare
4cfb758
to
438c78e
Compare
/werft run 👍 started the job as gitpod-build-jx-improve-billing-ui.4 |
48dd275
to
bf4cc13
Compare
bf4cc13
to
c09132b
Compare
Thanks, @jankeromnes! |
Aha, good catch @laushinka! This happens when the usage limit is But the |
How about when the limit is 0, instead of "Manage usage limit" we show "Set usage limit" and hide the percentage? |
I like this suggestion! But doesn't 0 currently mean "I want absolutely no usage"? (I.e. when it's set to 0, I believe Gitpod won't start any workspaces, and you'll see the "limit reached" notification.) Personally, I'd be happy with revisiting the semantics of |
Looking at this now! 👀 |
Ahh, right. No limit means unlimited usage, but limit zero means no usage. Then let's go with your initial suggestion 👍🏼 |
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.
Excited to see this feature coming together! ✨
Thanks, @jankeromnes!
Left some comments below, hopefully not too overwhelming. 🏁
Let's open follow-up issues for anything that feels out of the scope of this PR and could significantly increase the scope of the changes here. 🏓
</div> | ||
</div> | ||
)} | ||
{showUpgradeTeam && ( |
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.
issue: Probably not the best line to add this, but could we easily avoid the back-to-back duplicate spinners when loading the team billing page?
team-billing.mov
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.
Good catch! But I think it's not that easy to do away with them -- the first one is when we figure out whether to show the old Chargebee UI or the new Stripe UI, and the second one is while we actually load all the Stripe-relevant data.
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.
Or, we could simply decide not to show the initial spinner. I generally don't like leaving any "loading states" as a blank page ("wait, that's it? is it doing anything? is it broken?") but if it always transitions rapidly to the Stripe Billing UI, then maybe we could do without the first spinner. 💭
components/dashboard/src/components/UsageBasedBillingConfig.tsx
Outdated
Show resolved
Hide resolved
components/dashboard/src/components/UsageBasedBillingConfig.tsx
Outdated
Show resolved
Hide resolved
components/dashboard/src/components/UsageBasedBillingConfig.tsx
Outdated
Show resolved
Hide resolved
components/dashboard/src/components/UsageBasedBillingConfig.tsx
Outdated
Show resolved
Hide resolved
Woohoo, many thanks @gtsiolis for the extensive UX review! /hold so that I get a chance to address your comments and suggestions 👀 |
Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>
Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>
…gs pages (including billing pages)
f65f604
to
f9414d2
Compare
Phew. Okay I've now addressed most of the comments/suggestions, including fixing the I'm leaving as potential follow-up improvements:
With that, I think we're ready to merge this whenever we want! 🚀 It would be good to deploy this early next week, to support Early Access for Individuals. /unhold |
@@ -97,9 +97,12 @@ export default function Menu() { | |||
// Hide most of the top menu when in a full-page form. | |||
const isMinimalUI = inResource(location.pathname, ["new", "teams/new", "open"]); | |||
const isWorkspacesUI = inResource(location.pathname, ["workspaces"]); | |||
const isAccountUI = inResource(location.pathname, [ | |||
const isPersonalSettingsUI = inResource(location.pathname, [ |
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.
[random] inResource
is super misleading. I expected this to check if the pathname contains that resource, but it's actually checking if it starts with those resources.
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.
Thanks for calling this out @easyCZ! I must admit I only briefly skimmed this code while implementing my fix, but I agree about the name inResource
being super unclear and unhelpful. Do you have a better suggestion in mind? 🙂
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.
resourceStartsWith
or locationStartsWith
comes to mind. Mostly to communicate it's not just a substring match, but a starts with match
Description
Update billing page layout for usage-based teams and personal accounts.
Related Issue(s)
Fixes #13405
How to test
/billing
(your individual billing settings)Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide