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

Added confirmation dialog when expiring silences #993

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

conr
Copy link
Contributor

@conr conr commented Sep 14, 2017

Closes #883

Added to silence list view and individual silence view.

See:
test

@stuartnelson3 stuartnelson3 requested a review from w0rm September 14, 2017 13:33
@stuartnelson3
Copy link
Contributor

Thanks for the contribution! I assigned @w0rm to take a look, he should have some time soon.

Regarding the test failure: Once we're ready to merge, the last step is to run make build-all from the root dir. This compiles the elm code into javascript, and embeds the javascript into the alertmanager binary.

@conr
Copy link
Contributor Author

conr commented Sep 14, 2017

@stuartnelson3 Strange it's failing as I ran make build-all as per the contribution.md for the Alertmanager ui.

@w0rm
Copy link
Member

w0rm commented Sep 14, 2017

@Conorbro thanks for working on this issue!

I have two concerns:

  1. do we really need to use a library for this or we can have our own dialog component?
  2. showing a confirmation instead of a button or in a tooltip would be a better ux, because it doesn't block the whole ui

Because it's an improvement over what we have now, I think it can be merged, and then we could iterate on top.

@conr
Copy link
Contributor Author

conr commented Sep 14, 2017

@w0rm

Yes I agree, I wasn't sure whether introducing another library would be a good idea or not.

It's a fairly simple library so we could probably implement something similar ourselves with a confirmation/tooltip as you have suggested in the future.

@conr conr merged commit 2a0be5c into prometheus:master Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants