-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Alerting: Reorder new alert and export buttons #68418
Conversation
8985d83
to
c12cb7b
Compare
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.
LGTM!🎉
I added a non blocking comment.
Should we remove the |
@@ -235,6 +235,17 @@ const unifiedRoutes: RouteDescriptor[] = [ | |||
() => import(/* webpackChunkName: "AlertingRuleForm"*/ 'app/features/alerting/unified/RuleEditor') | |||
), | |||
}, | |||
{ |
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.
It seems to me the current number of routes is a bit confusing. We have
/alerting/new
/alerting/new/grafana
/alerting/new/cloud-alerting
/alerting/new/cloud-recording
What if we have/alerting/new/alerting
and/alerting/new/recording
?
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.
And what about Loki/Mimir? (currently cloud-alerting
), shouldn't we add a new route for it as well?
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 think both Grafana and Loki/Mimir should be under /alerting/new/alerting
.
My rationale is that you click Create Alert Rule
or More -> Create recording rule
so we have two different entry points. Additionally, if we remove the Recording rule
from the rule type selection, this routing change makes more sense. Yet it makes less sense if we do not remove the Recording rule
from the type selection.
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.
It sounds good to me, removed the button and changed the routes as you suggested 👍
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.
LGTM! one nit
</LinkButton> | ||
)} | ||
|
||
<Dropdown overlay={newMenu}> |
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 we hide the dropdown if none of canCreateGrafanaRules, canCreateCloudRules
, and canReadProvisioning
is true?
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 idea, done.
What is this feature?
Moves the new alert rule button to the top and creates a dropdown to display the options to create a recording rule and export all rules.
Now each new rule option has its own path: Grafana and Loki/Mimir are under
new/alerting
while recording rules can be accessed fromnew/recording
.Why do we need this feature?
To group buttons in a more clear way.
Who is this feature for?
All users
Which issue(s) does this PR fix?:
Fixes #68093
Fixes #68185