elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

[Rules] Replace alerting popover fields with standard EUI form controls #77549

Open sorenlouv opened 4 years ago

sorenlouv commented 4 years ago

A normal dropdown require a total of 2 clicks for making a change:

Example: ordinary dropdown:

The dropdown in alerting requires a total of 4 clicks for making the same change:

Example: dropdowns in alerting

Suggestion

We should use the standard EUI form controls. This will improve accessibility and user ergonomics

elasticmachine commented 4 years ago

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

mikecote commented 4 years ago

cc @mdefazio

mdefazio commented 4 years ago

I'm not sure this is specific to alerting—but moreso for the expression/popover experience in general. So this might need to be fixed on the EUI component level. I will bring it up with the team

sorenlouv commented 4 years ago

Agree, thanks for raising this to the team @mdefazio. Always seemed like a temporary workaround to put the select in a popover instead of opening it directly when clicking the expression.

cchaos commented 4 years ago

@chandlerprall This is possibly related to: https://github.com/elastic/eui/issues/3172

sorenlouv commented 4 years ago

@cchaos Definitely related. I should also point out that it requires two clicks to open the select. Should I close this issue and add that detail to the existing issue? Or create a new issue around this?

cchaos commented 4 years ago

Up to you how you want to track this. But you can make a note of the double click to open portion in the same issue linked above. Though, while I can replicate the double click to close on our docs site, I can't replicate the double click to open so we may need more information there.

sorenlouv commented 4 years ago

Though, while I can replicate the double click to close on our docs site, I can't replicate the double click to open so we may need more information there. @cchaos

You can replicate it in the EUI documentation:

A normal dropdown opens when clicking the selected value. In this case I'd expect the dropdown to open immediately when I click the expression. Instead it opens a flyout with the actual dropdown which the user then has to click to open the dropdown.

cchaos commented 4 years ago

Oic. That's not a bug but an implementation detail because the <EuiSelect> lives inside an <EuiPopover>. The expression isn't triggering the opening of a select input, it's triggering the opening of a popover containing a select input.

The reason for this example is to show that you can also have both parts of the expression be editable:

Screen Shot 2020-09-23 at 15 38 15 PM
sorenlouv commented 4 years ago

The expression isn't triggering the opening of a select input, it's triggering the opening of a popover containing a select input

I get that - and I think that's a UX issue :)

The reason for this example is to show that you can also have both parts of the expression be editable:

I'd expect "IS BELOW" to be a dropdown that opens immediately, and "100" to be editable in-place. That being said, for that particular use case it might be okay. However, for the example in my screenshot above I don't see any good reasons for making the user click twice. That's just making them do more work for the same action, isn't it?

sorenlouv commented 4 years ago

From an accessibility and user ergonomics perspective, vanilla EUI is superior - and personally I think it looks just as good:

gmmorris commented 3 years ago

I'd like to make sure this doesn't go stale :) - ideally we either agree it is a problem and decide whether it requires effort by Alerting / EUI, or we agree it's a non-issue and close it.

@cchaos - reading through the thread it sounds like it could be considered an EUI enhancement request, would you agree? Can we reroute this issue to become an EUI ER?

cchaos commented 3 years ago

I still back my statement that this is purely an implementation detail. EuiExpression does nothing itself with the popover but is purely a stylized block of text (or button). How you handle what exists in the popovers is dependent on context.

My guess is that EuiExpression is used here because it's much easier to scan and read and so that was prioritized over changing the selections. You could go with @sqren's suggestion, using pure input controls with prepend options. Or, you can pull the list of options out of the current select input and directly into the popover using EuiSelectable. And break apart each of the changeable parts of the expression statement to be their own selectable list/input that should get auto-focused on opening the popover.

Screen Recording 2021-07-22 at 02 46 42 PM

Here is a codesandbox example: https://codesandbox.io/s/misty-pond-748mq?file=/index.js of the example above.

sorenlouv commented 3 years ago

Here is a codesandbox example: codesandbox.io/s/misty-pond-748mq?file=/index.js of the example above.

This is perfect. Changing it to this would make me a very happy alerting user :)

gmmorris commented 3 years ago

Sounds good to me, thanks y'all.

@mikecote - I'm marking this on us, we can work with @mdefazio to see if @cchaos 's suggestion works for us .

gmmorris commented 3 years ago

Switching this from RulesManagement to RuleTypes as this is implementation specific to individual RuleTypes. Whatever change we make here will only apply to StackRules (ES Query and Index Threshold). We can either open follow up issues for other teams on their individual RuleTypes or, if this is a simple enough change, offer to update the other team's UX to align with ours, depending on LOE (I trust y'all to use your own judgment on that).

elasticmachine commented 2 years ago

Pinging @elastic/response-ops (Team:ResponseOps)