-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(ui): Enhance resource creation experience when limits are reached #19103
Conversation
95c948e
to
6059f93
Compare
Just wanna say how great an enhancement this is |
export const extractRulesMax = (limits: LimitsState): number => { | ||
return get(limits, 'rules.maxAllowed') || Infinity | ||
} | ||
export const extractBlockedRules = (limits: LimitsState): string[] => { | ||
return get(limits, 'rules.blocked') || [] | ||
} | ||
|
||
export const extractEndpointsLimits = (limits: LimitsState): LimitStatus => { | ||
return get(limits, 'endpoints.limitStatus') |
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.
should these return some defaults?
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.
I don't think so. The defaults are set to LimitStatus.OK
so there's always a value when these gets are called.
@@ -13,6 +13,7 @@ export type OverlayID = | |||
| 'telegraf-output' | |||
| 'switch-organizations' | |||
| 'create-bucket' | |||
| 'asset-limit' | |||
|
|||
export interface OverlayParams { | |||
[key: string]: string |
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.
I really don't like that these params are actually required for the overlay to load properly, but they are not properly typed. I know you didn't write this, and I don't think you need to fix this in this PR, but it makes me worry :(
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.
Maybe we should open another tech debt issue for this.
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- I just had a few comments here and there but all are in the realm of style suggestions!
6059f93
to
d715fff
Compare
Change rate-limited create asset experience.
OLD EXPERIENCE
Create asset button disabled when asset limit is reached for Dashboards, Tasks, & Buckets
Trying to create a Check fails with an error after going though the process of configuring it
Trying to create a Rule fails silently with only a console error
NEW EXPERIENCE
Create asset button is always enabled
When user clicks to create asset they are presented with an upgrade overlay