Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding functionality to associate existing detector with a visualization #484
Adding functionality to associate existing detector with a visualization #484
Changes from 4 commits
2b74e3f
bfb9c69
7e24cd9
bcaa084
1a28212
fa45fd8
15dc68d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 have a setup alert button on existing AD detector overview page, is this how we currently check if alerting exist? also what does
ALERTING_TRIGGER_AD_ID
mean here?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 actually don't have this logic on the setup alert button in AD overview page. If there is no alerting plugin, the button still shows and it just sends to a broken page.
ALERTING_TRIGGER_AD_ID
is the trigger that David set up on the alerting code to open up the relevant flyout. If the alerting trigger doesn't exist then it means there is no alerting plugin or potentially something else going on but regardless it means the flyout for creating a monitor on top of a detector wont be openedThere 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 we used to have a check here. Regardless, this is bad UX. Can you open an issue on that. We can probably keep it broad, like "clean up how to detect alerting plugin is installed"
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.
Opened an issue #493
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.
here's a way to make the toast wider:
className: "successToast",
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.
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, made the fix
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.
Similar to the conditional on line 830, we can do the same here for these 2 exclusive scenarios.
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.
mode can also be 'associated'
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.
What is the difference between
existing
andassociated
? I'm thinking if we can have these in constants with details about each scenario it would be helpful.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.
existing is the one with the button to associate an existing detector flyout and associated is the flyout with the list of all currently associated detectors with ability to unlink. I can add some comments and constants, will do that in my final commit after fixing the dependency issue