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

Alerting: Add more context to delete modals #69244

Merged
merged 4 commits into from
May 31, 2023

Conversation

gillesdemey
Copy link
Member

@gillesdemey gillesdemey commented May 30, 2023

What is this feature?

Adds additional context to the delete modal so the user is aware of what they're asked to delete.

Affected modals in this PR:

  • Delete alert group
  • Delete alert

@gillesdemey gillesdemey added this to the 10.0.x milestone May 30, 2023
@gillesdemey gillesdemey requested review from a team and brendamuir May 30, 2023 07:52
@dhalachliyski
Copy link

Could you reverse the order of the buttons so that the destructive button is on the right?
Also, how about instead of "Yes, delete policy", just have "Delete"?

@brendamuir
Copy link
Contributor

Do we need the info on the matchers?
Could we just say "Are you sure you want to delete this notification policy?"
If not -- "Are you sure you want to delete this policy with the following label matchers?"

@gillesdemey
Copy link
Member Author

I think whether we need the info or not is debatable; the tree can be nested so any context is helpful to make sure the user is deleting the right policy.

@dhalachliyski
Copy link

dhalachliyski commented May 30, 2023

On the label matching topic:
What if don't call them out specifically, but instead just say "Are you sure you want to delete this notification policy?" as suggested. Then we visualize the policy not just by the matchers, but by a full width rectangular container that has the matchers left-aligned, and the contact point right-aligned (kind of what is being done currently for the Routing Preview)? The we'll also not need the horizontal line.
At the end we call in just a policy, but we provide the most distinctive things - matchers and contact point.

@gillesdemey
Copy link
Member Author

Yea I think that would work better, but those components are a work-in-progress and I'd rather not duplicate our efforts there; I'll revert the changes for the notification policies modal for now and that way we can at least merge some incremental improvements.

Copy link
Contributor

@konrad147 konrad147 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gillesdemey gillesdemey enabled auto-merge (squash) May 30, 2023 13:54
@gillesdemey gillesdemey merged commit deb13ef into main May 31, 2023
6 checks passed
@gillesdemey gillesdemey deleted the alerting/delete-modal-context branch May 31, 2023 09:47
@zerok zerok modified the milestones: 10.0.x, 10.0.1, 10.1.x Jun 20, 2023
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants