-
Notifications
You must be signed in to change notification settings - Fork 8.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
Renamed button and dropdown items in headers (apm, logs, metrics and uptime) from alerts to rules #100918
Renamed button and dropdown items in headers (apm, logs, metrics and uptime) from alerts to rules #100918
Conversation
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'm not sure why the kibana-telemetry or the kibana-core team is pinged i dont see any relevant files to the team. localization changes lgtm.
getRuleDetailsHref={ruleDetailsNavigation?.href} | ||
onRuleDetailsClick={ruleDetailsNavigation?.onClick} | ||
>>>>>>> master |
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 believe this git conflict diff is here by mistake 😬
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.
Seems like I missed this one. Thanks for spotting that.
@@ -67,7 +67,7 @@ export const ToggleAlertFlyoutButtonComponent: React.FC<Props> = ({ | |||
> | |||
<FormattedMessage | |||
id="xpack.uptime.navigateToAlertingButton.content" | |||
defaultMessage="Manage alerts" | |||
defaultMessage="Manage rules" |
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.
node scripts/i18n_check --fix
should fix any removed ids. our extraction tools automatically detect changed messages for retranslation so devs dont ahve to worry about it.
Moved this to a separate issue: #102485
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 -- we should change the associated method names too so we don't get very confused later but we can do that separately.
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 📏 📐
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.
A few minor questions - LMK your thoughts.
@@ -114,7 +114,7 @@ export const ToggleAlertFlyoutButtonComponent: React.FC<Props> = ({ | |||
}, | |||
{ | |||
id: ALERT_CONTEXT_SELECT_TYPE_PANEL_ID, | |||
title: 'create alerts', | |||
title: 'Create rules', |
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.
We should translate this title.
We can add the string to the list of translations in the dedicated file, and reference it here.
title: 'Create rules', | |
title: i18n.translate('xpack.uptime.alerts.createRulesPanel.title', { defaultMessage: 'Create rules' }), |
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.
Thank you, I will add that.
@@ -8,11 +8,11 @@ | |||
import { i18n } from '@kbn/i18n'; | |||
|
|||
export const ENABLE_STATUS_ALERT = i18n.translate('xpack.uptime.monitorList.enableDownAlert', { | |||
defaultMessage: 'Enable status alert', | |||
defaultMessage: 'Enable rule', |
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.
Do we lose some context here? Can we call it Enable status rule
or Enable status check rule
?
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 understand what you mean, I had the same thought but was wondering if it is ok as the column header says Status alert
.
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, thanks for doing this.
keeping kibana CI Happy can be a challenge and who doesn't love a designer who can also write code :)
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…uptime) from alerts to rules (#100918) (#102893) Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co> Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com> Co-authored-by: Steph Milovic <stephanie.milovic@elastic.co> Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co> Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com> Co-authored-by: Steph Milovic <stephanie.milovic@elastic.co>
Summary
As we updated the terminology around alerts, this is the PR updating our "Alerts" buttons in the headers and the dropdowns they trigger.
Logs
Metrics
APM
Uptime
Checklist
Delete any items that are not applicable to this PR.
For maintainers