Skip to content
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

Cleaning out Incident word from frontend #704

Merged
merged 11 commits into from
Feb 8, 2023

Conversation

Ukochka
Copy link
Contributor

@Ukochka Ukochka commented Oct 24, 2022

What this PR does:

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated

@joeyorlando
Copy link
Contributor

hi there @Ukochka 👋
does this PR need a review/should it be merged? or should we close it?

@Ukochka Ukochka requested a review from a team February 3, 2023 15:11
@Ukochka Ukochka added no changelog pr:no public docs Added to a PR that does not require public documentation updates labels Feb 3, 2023
@@ -63,7 +63,7 @@ const IncidentMatcher = observer((props: IncidentMatcherProps) => {
<div className={cx('columns')}>
<div className={cx('incident-list')}>
<Text.Title className={cx('title')} level={5}>
Matching Incidents
Matching Alert groups
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the "g" in "groups" also be capitalized if we're going to capitalize "Alert"?

@@ -31,7 +31,7 @@
"includes": [
{
"type": "page",
"name": "Incidents",
"name": "Alert groups",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same w/ capitalization here.

label="Require resolution note when resolve incident"
description={`Once user clicks "Resolve" for an incident they are require to fill a resolution note about the incident`}
label="Require resolution note when resolve alert group"
description={`Once user clicks "Resolve" for an alert group they are require to fill a resolution note about the alert group`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description={`Once user clicks "Resolve" for an alert group they are require to fill a resolution note about the alert group`}
description={`Once user clicks "Resolve" for an alert group, they will be required to fill in a resolution note about the alert group`}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ same here

@@ -50,8 +50,8 @@ class SettingsPage extends React.Component<SettingsPageProps, SettingsPageState>
<div className={cx('settings')}>
<Field
loading={!teamStore.currentTeam}
label="Require resolution note when resolve incident"
description={`Once user clicks "Resolve" for an incident they are require to fill a resolution note about the incident`}
label="Require resolution note when resolve alert group"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label="Require resolution note when resolve alert group"
label="Require a resolution note when resolving alert groups"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ good mention

Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider talking to the docs team (or maybe we have a copy-writing team?) regarding capitalization consistency (ex. Alert group vs alert group vs Alert Group). We have instances of all three throughout the UI.

@@ -395,7 +395,7 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState>
</li>
))}
</ul>
<Field label="Leave a resolution note" description="Will also show up in the thread of incident in Slack">
<Field label="Leave a resolution note" description="Will also show up in the thread of alert group in Slack">
Copy link
Member

@teodosii teodosii Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be

show up in the thread of the alert group
or
should up in the thread of an alert group ?

Depending on usage, I'm not sure to which alert group we're referring to in this case. It is not correct as it is right now

Copy link
Member

@teodosii teodosii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved but please add those changes.

@teodosii
Copy link
Member

teodosii commented Feb 6, 2023

This should also have the change in the changelog I believe, it's good to keep track of all the frontend changes this way.

@Ukochka Ukochka merged commit 21691d6 into dev Feb 8, 2023
@Ukochka Ukochka deleted the cleaning-incident-word-from-oncall branch February 8, 2023 16:07
brojd pushed a commit that referenced this pull request Sep 18, 2024
**What this PR does**:

**Which issue(s) this PR fixes**:

**Checklist**
- [ ] Tests updated
- [ ] Documentation added
- [x] `CHANGELOG.md` updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants