Skip to content

[Uptime] Filters in create alert flyout#64753

Merged
shahzad31 merged 27 commits intoelastic:masterfrom
shahzad31:filter-in-flyout
May 5, 2020
Merged

[Uptime] Filters in create alert flyout#64753
shahzad31 merged 27 commits intoelastic:masterfrom
shahzad31:filter-in-flyout

Conversation

@shahzad31
Copy link
Copy Markdown
Contributor

@shahzad31 shahzad31 commented Apr 29, 2020

Summary

Fixes: elastic/uptime#163
Displayed currently selected filter in create alert and added ability to change filters

image

@shahzad31 shahzad31 requested a review from justinkambic April 30, 2020 12:45
@shahzad31 shahzad31 marked this pull request as ready for review April 30, 2020 13:37
@shahzad31 shahzad31 requested a review from a team as a code owner April 30, 2020 13:37
@shahzad31 shahzad31 self-assigned this Apr 30, 2020
@shahzad31 shahzad31 added release_note:enhancement v7.8.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Apr 30, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Copy Markdown
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

I had a few minor comments about code. In addition there were a few functionality-related things I wanted to voice.

First - in a case where the user uses the statusFilter on the Overview page to filter by up, and then uses the search field in the AddAlert flyout to select a monitor whose status is down (or the reverse), our new monitor counter will show 0 monitors.

image

Another thing to consider is how we want the page to behave when other filtering criteria changes the values we display for filtering. For instance, I chose a couple of ports to filter on, then updated the search text, and now I seem to be unable to deselect one of the ports:

image

Lastly, I'm having some difficulty getting the Create alert button at the bottom of the flyout to enable. This might be related to some validation issue signaling the flyout to not allow the user to submit.

Copy link
Copy Markdown
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

Copy link
Copy Markdown
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Ok, this LGTM now! The functional concerns I mentioned have been resolved, and there's a follow-up PR #65049 to address the problem with filters/status filter clashing.

@andrewvc
Copy link
Copy Markdown
Contributor

andrewvc commented May 5, 2020

@shahzad31 I hope you don't mind, I noticed there were a couple conflicts in this PR so I pushed a commit resolving them 🤞

@andrewvc
Copy link
Copy Markdown
Contributor

andrewvc commented May 5, 2020

@shahzad31 I also pushed a tiny commit fixing the casing of the word Tag.

Copy link
Copy Markdown
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

@shahzad31 I'm LGTM on this if my last two commits look good to you

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@shahzad31 shahzad31 merged commit 6d9c59d into elastic:master May 5, 2020
@shahzad31 shahzad31 deleted the filter-in-flyout branch May 5, 2020 09:35
shahzad31 added a commit to shahzad31/kibana that referenced this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.8.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add filter dropdowns to alerting dialog

6 participants