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

[Controls] Migrate range slider to EuiDualRange and make styling consistent across all controls #162651

Merged
merged 53 commits into from
Aug 16, 2023

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Jul 27, 2023

Closes #140135
Closes #159724

Summary

Changes to range slider

This PR migrates the range slider control to use the EuiDualRange component, which both (a) removes a bunch of range slider code , thus improving DX, and (b) improves the usability of the control by simplifying the different states and making the slide animation much smoother:

Before After
Aug-02-2023 09-19-26 Aug-03-2023 09-10-30

Note that, due to the fact that migrating to the EuiDualRange component means we no longer control the opening/closing of the popover, this makes the "selections that extend beyond the current min/max of the data" scenario slightly more difficult to handle. Specifically, we need to decide the best time to recalculate the min/max of the slider as the user's selections change.

The benefit of having control over the popover opening and closing was that we could calculate the range slider's min/max values when the popover opened and lock them in place so that they stayed static as the user dragged:


However, because the EuiDualRange handles the behaviour of the popover and we therefore do not know when it opens/closes, it is not currently possible to fully replicate this old behaviour. We have two main options:

  1. Recalculate the min/max of the range slider the user drags, which gives a "sliding" effect:

  2. Debounce this min/max calculation - with this, we avoid the "sliding" effect shown above, and the min/max values only get adjusted when the pin has been static for a long enough period of time (specifically, 750ms) or when the pin is dropped (i.e. the user lets go of the mouse):

Ultimately, I went with option 2 in this PR. As a future enhancement, we could replicate the old behaviour by adding some sort of onPopoverClosed or onPopoverOpened logic to the EuiDualRange component - however, this would need discussion with the EUI team to ensure that this is the best way to handle it and not too specific to our use case. Since EUI changes take awhile to propagate to KIbana, it's not worth holding up this PR even further.

Consistency with other control types

To keep things consistent, this PR also switches both the options list and time slider controls to use the EuiInputPopover component and removes the redundant control titles from the popover header:

Before After
image image
image image
image image

Checklist

For maintainers

@Heenawter Heenawter added release_note:enhancement Feature:Input Control Input controls visualization enhancement New value added to drive a business result Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. backport:skip This commit does not require backporting Project:Controls labels Jul 27, 2023
@Heenawter Heenawter self-assigned this Jul 27, 2023
@Heenawter Heenawter force-pushed the switch-to-dual-range_2023-05-15 branch from 211577a to 4f9abae Compare July 27, 2023 18:26
@Heenawter Heenawter added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement enhancement New value added to drive a business result labels Jul 27, 2023
@Heenawter Heenawter force-pushed the switch-to-dual-range_2023-05-15 branch 9 times, most recently from c6e587e to 9583ae6 Compare August 1, 2023 17:54
Heenawter added a commit to Heenawter/kibana that referenced this pull request Aug 2, 2023
@Heenawter Heenawter added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 14, 2023
@Heenawter
Copy link
Contributor Author

Heenawter commented Aug 14, 2023

WRT the "Security Solution Cypress tests, burning changed specs" failures... I'm investigating, and it seems like both of the alerts get acknowledged before the options list has time to check the counts in the "Alert list is updated when the alerts are updated" test:

image

Whereas, when running it locally (where it passes reliably), there is one "Open" alert and one "Acknowledged" alert, like so:

image

Ideally, when the test first loads, both alerts should be Open - then, the markAcknowledgedFirstAlert() will get it into the expected state. But something seems to be happening where both alerts get acknowledged before the options list has time to verify its values.

So, this doesn't seem like this is actually a problem with the options list, and is instead something funky going on with the test itself... 🤔 Also, the second attempt doesn't seem to get reset properly, so you just end up in a no data state - i.e. it always fails on the retry, which isn't ideal.

@andreadelrio
Copy link
Contributor

@Heenawter seems like the fix on the EUI side came much sooner than we expected. Should we hold this off til that is released into Kibana? It shouldn't take too long.

@logeekal
Copy link
Contributor

@Heenawter

since it is holiday is germany, for now, I have disabled the problematic tests and raised a bug for it : #163920

Now, all suites are passing and security tests should not block you.

@Heenawter
Copy link
Contributor Author

Heenawter commented Aug 15, 2023

@andreadelrio While that PR does improve the options list ever so slightly (it will basically function the same as it does now in this PR, but (a) the code will be a bit cleaner, because we'll no longer need the resize observer and (b) we could have a default downLeft positioning rather than the current center positioning), it still does not address the issue with the range slider being quite cramped in our small size. I brought this up with the EUI team, and they said that this is something they can add to the EuiDualRange - but we will have to wait until that PR also gets merged (plus the corresponding EUI update).

I personally think it is still best to merge this PR as-is and wait until we can fix both the options list and the range slider in the follow up :)

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design changes LGTM. Thanks a lot!

@Heenawter Heenawter requested a review from a team as a code owner August 15, 2023 18:23
Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

LGTM!!

@logeekal
Copy link
Contributor

@Heenawter ,

A heads up regarding failing Serverless Security Cypress Tests. This PR will get rid of these errors as soon as it is merged. 🤞

Copy link

@codearos codearos left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Cypress Tests #3 / Exceptions viewer read only "before each" hook for "Cannot take actions on exception" "before each" hook for "Cannot take actions on exception"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 160 156 -4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 197.2KB 195.0KB -2.2KB
securitySolution 15.7MB 15.7MB +342.0B
total -1.9KB

History

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

cc @Heenawter

@Heenawter Heenawter merged commit 75d27ad into elastic:main Aug 16, 2023
@Heenawter Heenawter deleted the switch-to-dual-range_2023-05-15 branch August 16, 2023 17:27
Heenawter added a commit that referenced this pull request Sep 22, 2023
Closes #164375

## Summary

This PR wraps up #162651 by fully
migrating to the `EuiInputPopover` for all controls - specifically, this
is made possible by the new `panelMinWidth` prop, which makes it so that
the popover can now extend past the size of the input **while
maintaining** the expected positioning.

| Before | After |
|--------|--------|
| The popover was centered underneath the control on the smallest
size:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/e2814ee2-6df6-47d6-925e-9f97cb8be2a5)
| The popover is left-aligned with the start of the input and expands to
the
right:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/7c698ef0-1534-43b6-ac95-9ae95f1c7613)
|
| The range slider popover could not extend past the control width,
regardless of how small that
was:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/12e33967-b616-4f0a-9ded-4374d65a51b2)
| The range slider popover now also has a minimum width, which makes it
more useable on the smallest
size:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/2fb844db-8f5d-44d8-a6dc-c9cb95d5a4ea)
|

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
joemcelroy pushed a commit to joemcelroy/kibana that referenced this pull request Sep 25, 2023
Closes elastic#164375

## Summary

This PR wraps up elastic#162651 by fully
migrating to the `EuiInputPopover` for all controls - specifically, this
is made possible by the new `panelMinWidth` prop, which makes it so that
the popover can now extend past the size of the input **while
maintaining** the expected positioning.

| Before | After |
|--------|--------|
| The popover was centered underneath the control on the smallest
size:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/e2814ee2-6df6-47d6-925e-9f97cb8be2a5)
| The popover is left-aligned with the start of the input and expands to
the
right:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/7c698ef0-1534-43b6-ac95-9ae95f1c7613)
|
| The range slider popover could not extend past the control width,
regardless of how small that
was:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/12e33967-b616-4f0a-9ded-4374d65a51b2)
| The range slider popover now also has a minimum width, which makes it
more useable on the smallest
size:<br><br>![image](https://github.com/elastic/kibana/assets/8698078/2fb844db-8f5d-44d8-a6dc-c9cb95d5a4ea)
|

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Input Control Input controls visualization impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Controls release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.10.0
Projects
None yet
9 participants