-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat: Comparison based alerts #4818
Conversation
cf31d8e
to
5e08468
Compare
eeb5a2e
to
2b7bce1
Compare
2b7bce1
to
0649150
Compare
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.
Backend changes look good :)
1a028cb
to
6d2f043
Compare
d25a09d
to
f662c6b
Compare
8df4e87
to
8f67404
Compare
Code – So far I've just skimmed it, but I'm seeing a few leftover console logs FYI. |
6321321
to
787c7de
Compare
Thanks for the review. Have fixed 1,4 and 7. The duration and label mapping was incorrect (delta was incorrectly 0). |
UX/Product Check on @ericpgreen2's first pass1. Comparing with pill is now displaying previous time period 2. Keyboard "Escape" key doesn't trigger "close without saving" dialog anymore 3. Looks like the column spillover is still there 4. % change from undefined is now fixed 5. Adding a new criteria now preloads with a measure (instead of displaying the measure label). 6. Email recipients still initialized with extra blank email input field 7. Criteria readout looks good now New UXQA1. For long measure names, looks like there's spillover in the criteria dropdown box 2. Selecting first measure from Alert data dropdown will autopopulate measure selection in Criteria step. Selecting other measures will not autopopulate measure selection 3. [URGENT] You have to click next button twice on Criteria step to get to Delivery step 4. Shouldn't say "% change from from previous day". Remove one of the "from" |
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.
Approved 👍. For those UXQA items that are not in scope of this PR, @ericokuma is adding to the Alerts UXQA Notion doc.
c983f98
to
4606c09
Compare
Fixed the new items. Found the issue for incorrect column height. Fixed it when the column is not in the 1st place. For the case where it is in 1st place, the code is used in dimension table as well, so will do a follow up after confirming why we have that in dimension table. |
closes #4544
Uses #4793 to add comparison based alerts on the MetricsViewAggregation API