[Controls] Move "clear selections" to hover action #159526

Heenawter · 2023-06-12T22:00:29Z

Summary

This PR moves the "clear selections" button for all controls (options list, range slider, and time slider) from inside their respective popovers to a general hover action - this not only saves users a click for this common interaction (which has actually been brought in user feedback up as a downside of the current controls compared to the legacy controls), it also allows us to fully move forward with migrating the range slider control to the EuiDualRange component. This will be done in a follow up PR, which should both (1) clean up our range slider code significantly and (2) fix the bug discussed here. The related issue can be tracked here, since we might not be able to get to it right away.

This "clear selections" action is available in both view and edit mode, like so:

	Edit mode	View mode
Range slider
Options list
Time slider

You may notice in the above screenshots that the "delete" action is now represented with a red trash icon rather than a red cross, and the tooltip text was also changed to use the word "Delete" rather than the word "Remove" - these changes were both made to be more consistent with the "Delete panel" action available on dashboards:

Delete control - Before	Delete control - After	Delete panel

Beyond these changes, I also made a few quick changes to the time slider control, including:

Fixing the appearance so that the background is once again white, as described here
Adding comparison logic so that clearing selections no longer causes unsaved changes unnecessarily, as described here

Videos

Before

Screen.Recording.2023-06-13.at.10.41.15.AM.mov

After

Screen.Recording.2023-06-13.at.11.31.55.AM.mov

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-06-14T15:11:56Z

src/plugins/controls/common/control_group/control_group_panel_diff_system.ts

+      return (
+        Boolean(isAnchoredA) === Boolean(isAnchoredB) &&
+        Boolean(startA) === Boolean(startB) &&
+        startA === startB &&
+        Boolean(endA) === Boolean(endB) &&
+        endA === endB
+      );


This fixes a bug where resetting the selections back to "undefined" / empty would cause unsaved changes:

Before:

Screen.Recording.2023-06-14.at.10.35.23.AM.mov

After:

Screen.Recording.2023-06-14.at.10.36.53.AM.mov

Heenawter · 2023-06-14T15:13:46Z

src/plugins/controls/public/control_group/component/control_group_component.tsx

@@ -82,6 +82,7 @@ export const ControlGroup = () => {
        });
      }
    }
+    (document.activeElement as HTMLElement)?.blur();


This fixes this bug where the drag handler was retaining focus after it was dropped, which caused the floating actions to remain on the screen after moving a control until the user clicked somewhere to manually remove the focus:

Before:

Screen.Recording.2023-06-14.at.10.42.44.AM.mov

After:

Screen.Recording.2023-06-14.at.10.41.58.AM.mov

Heenawter · 2023-06-14T15:14:12Z

src/plugins/controls/public/control_group/control_group_strings.ts

+    clearAction: {
+      [OPTIONS_LIST_CONTROL]: {
+        getClearButtonTitle: () =>
+          i18n.translate('controls.controlGroup.floatingActions.optionsList.clearTitle', {
+            defaultMessage: 'Clear selections',
+          }),
+      },
+      [RANGE_SLIDER_CONTROL]: {
+        getClearButtonTitle: () =>
+          i18n.translate('controls.controlGroup.floatingActions.rangeSlider.clearTitle', {
+            defaultMessage: 'Clear range selection',
+          }),
+      },
+      [TIME_SLIDER_CONTROL]: {
+        getClearButtonTitle: () =>
+          i18n.translate('controls.controlGroup.floatingActions.timeSlider.clearTitle', {
+            defaultMessage: 'Clear time selection',
+          }),
+      },


Not sure if we want the copy to be unique for each control type:

Each control had unique tooltips when this "clear" action was unique to their respective popovers, so I copied this logic over for this.... but I wonder if simply "clear selections" would work regardless of the control type?

cc @gchaps @andreadelrio

I would just have it say Clear.

Hmm... Maybe "Clear control" so it's more consistent with the other tooltips?

Unless we want to change the tooltip for those actions, too - so it would simply be "Clear", "Edit", and "Delete" for the tooltips respectively.

I would go the route of simplifying and removing the word control from all the tooltips.

I like it - it looks super clean 👍 Thanks!

Heenawter · 2023-06-14T15:14:53Z

src/plugins/controls/public/time_slider/components/index.scss

@@ -33,7 +33,7 @@
  }

  .euiText {
-    background-color: $euiFormBackgroundColor;
+    background-color: $euiFormBackgroundColor !important;


Before After

Heenawter · 2023-06-14T16:51:32Z

src/plugins/controls/public/control_group/actions/delete_control_action.tsx

@@ -60,7 +60,7 @@ export class DeleteControlAction implements Action<DeleteControlActionContext> {
    if (!embeddable.parent || !isControlGroup(embeddable.parent)) {
      throw new IncompatibleActionError();
    }
-    return 'cross';
+    return 'trash';


As noted in the PR description, this change was made to be more consistent with the words + icons we use when deleting panels from the dashboard:

Delete control - Before Delete control - After Delete panel

elasticmachine · 2023-06-14T16:52:42Z

Pinging @elastic/kibana-presentation (Team:Presentation)

…-ref HEAD~1..HEAD --fix'

src/plugins/controls/public/time_slider/components/time_slider.tsx

nreese · 2023-06-15T17:49:48Z

src/plugins/controls/public/time_slider/embeddable/time_slider_embeddable.tsx

@@ -354,6 +352,10 @@ export class TimeSliderControlEmbeddable extends Embeddable<
      .format(this.getState().componentState.format);
  };

+  public clearSelections() {
+    this.onTimesliceChange();


There is something going on with Maps embeddable panels where clearing time slider does not refetch map with correct data. Instead, map still shows data from the previously selected time slice. Maybe you need to pass [timeRangeMin, timeRangeMax] into onTimesliceChange? Or debug the issue in map embeddable

I have found the problem in the map_embeddable. You need to pass clearTimeslice to setQuery, so change MapEmbeddable._dispatchSetQuery to the below implemenation. Otherwise, setQuery will pull timeslice from state when its passed in as undefined here.

_dispatchSetQuery({ forceRefresh }: { forceRefresh: boolean }) { this._savedMap.getStore().dispatch<any>( setQuery({ filters: this._getInputFilters(), query: this.input.query, timeFilters: this.input.timeRange, timeslice: this.input.timeslice ? { from: this.input.timeslice[0], to: this.input.timeslice[1] } : undefined, clearTimeslice: this.input.timeslice === undefined, forceRefresh, searchSessionId: this._getSearchSessionId(), searchSessionMapBuffer: getIsRestore(this._getSearchSessionId()) ? this.input.mapBuffer : undefined, }) ); }

Oooh yes that makes total sense - thank you! I completely forgot to test the Maps use of the time slider, so thank you for catching that 🙇

Fixed in b3ccbc8 👍

…hover-action_2023-06-12

nreese

LGTM
code review and tested in chrome

nreese · 2023-06-15T19:08:09Z

src/plugins/controls/public/types.ts

@@ -46,6 +47,12 @@ export type ControlEmbeddable<
  renderPrepend?: () => ReactNode | undefined;
 };

+export abstract class ClearableControlEmbeddable<


Nit, do you see use cases where control is not clearable? How about just making clearSelections part of ControlEmbeddable interface? I am not convinced that a new interface is needed.

Each embeddable extended the generic Embeddable class and not ControlEmbeddable (which is just a type at the moment and not a class/interface) before this. I can try to refactor things to make each control extend ControlEmbeddable instead, cause I think you're right that the current implementation isn't very clear....

I do very much like the decoupling here that comes with having the clear functionality defined outside of the ControlEmbeddable Interface. That said, I'm wondering about the specific choice to use an Abstract class here instead of an interface? IMO an interface accomplishes the same thing with less boilerplate and more type safety. You could do this with:

An interface IClearableControl that defines the clear selections method

A type guard that checks for the presence of the clear selections method.

Run the embeddable through the type guard in the isCompatible method of the action rather than using instanceof.

that way, theoretically any control can just determine if it's clearable or not by implementing that interface.

more type safety

I specifically went with an abstract class so that each control type would throw a typescript error if clearSelections wasn't implemented... But I think you're correct that this may have been overkill 👀 I can achieve the same behaviour by having each control embeddable implement the IClearableControl interface I believe. I can try this solution out.

That is true yes! Typescript interfaces are quite flexible, and if you implement the interface you'll need to provide the function or else there'll be an error.

Changed to this solution in 31e64af - I think it's cleaner and more inline with how we do other typing 👍

…-ref HEAD~1..HEAD --fix'

kibana-ci · 2023-06-19T16:41:15Z

💚 Build Succeeded

Buildkite Build
Commit: b97efa9

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	158	160	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`controls`	299	301	+2

Async chunks

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

id	before	after	diff
`controls`	190.3KB	199.8KB	+9.5KB
`maps`	2.7MB	2.7MB	+45.0B
total			+9.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`controls`	15	16	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`controls`	37.0KB	37.5KB	+473.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	306	308	+2

async chunk count

id	before	after	diff
`controls`	7	8	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+6

History

💚 Build #135790 succeeded fc117ae
💔 Build #135763 failed eed68ee
💚 Build #135701 succeeded 3893cf5
💚 Build #135322 succeeded 7910151
💚 Build #134959 succeeded 82efb91
💔 Build #134938 failed c29530a

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

cc @Heenawter

andreadelrio

Design changes LGTM. This is a very exciting change! While it is true that now the clear button is one less click away it is also further away from users when they're making their selections in the popover. I think it's a possibility that this will come up as feedback (users wanting to have the clear action in two places) but let's wait and see.

Heenawter self-assigned this Jun 12, 2023

Heenawter force-pushed the controls-clear-hover-action_2023-06-12 branch 2 times, most recently from 66ad9d3 to 50e865f Compare June 13, 2023 17:45

Heenawter added 6 commits June 13, 2023 11:57

Add clear selections action

32d71dd

Remove clear buttons in popovers

57de656

Add time slider

0a2e174

Fix time slider appearence

ebe9c69

Fix functional tests

626fb87

Clean up + force blur of button on click

c29530a

Heenawter force-pushed the controls-clear-hover-action_2023-06-12 branch from 50e865f to c29530a Compare June 13, 2023 17:57

Hide actions on drag end

82efb91

Heenawter mentioned this pull request Jun 13, 2023

[Dashboard] [Controls] Hide hover actions again after drag and drop #154238

Closed

Heenawter commented Jun 14, 2023

View reviewed changes

Heenawter mentioned this pull request Jun 14, 2023

[Controls] Fully migrate range slider to EuiDualRange component #159724

Closed

Heenawter commented Jun 14, 2023

View reviewed changes

Heenawter marked this pull request as ready for review June 14, 2023 16:52

Heenawter requested review from a team as code owners June 14, 2023 16:52

Update tooltip strings

72f7638

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

7910151

…-ref HEAD~1..HEAD --fix'

nreese self-requested a review June 15, 2023 15:21

nreese reviewed Jun 15, 2023

View reviewed changes

src/plugins/controls/public/time_slider/components/time_slider.tsx Outdated Show resolved Hide resolved

Remove onClear prop from time slider

3893cf5

nreese reviewed Jun 15, 2023

View reviewed changes

Add maps fix that Nathan found

b3ccbc8

Heenawter requested a review from a team as a code owner June 15, 2023 18:53

Merge branch 'main' of github.com:elastic/kibana into controls-clear-…

eed68ee

…hover-action_2023-06-12

nreese approved these changes Jun 15, 2023

View reviewed changes

Heenawter and others added 3 commits June 15, 2023 14:02

Fix up typing

31e64af

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

fc117ae

…-ref HEAD~1..HEAD --fix'

Merge branch 'main' into controls-clear-hover-action_2023-06-12

b97efa9

andreadelrio approved these changes Jun 20, 2023

View reviewed changes

Heenawter merged commit f1dc1e1 into elastic:main Jun 20, 2023

Heenawter deleted the controls-clear-hover-action_2023-06-12 branch June 20, 2023 22:53

kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Controls] Move "clear selections" to hover action #159526

[Controls] Move "clear selections" to hover action #159526

Heenawter commented Jun 12, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

andreadelrio Jun 14, 2023

Heenawter Jun 14, 2023

andreadelrio Jun 14, 2023

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023

elasticmachine commented Jun 14, 2023

nreese Jun 15, 2023 •

edited

Loading

nreese Jun 15, 2023 •

edited

Loading

Heenawter Jun 15, 2023

Heenawter Jun 15, 2023

nreese left a comment •

edited

Loading

nreese Jun 15, 2023

Heenawter Jun 15, 2023 •

edited

Loading

ThomThomson Jun 15, 2023

Heenawter Jun 15, 2023 •

edited

Loading

ThomThomson Jun 15, 2023

Heenawter Jun 15, 2023

kibana-ci commented Jun 19, 2023

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

andreadelrio left a comment

[Controls] Move "clear selections" to hover action #159526

[Controls] Move "clear selections" to hover action #159526

Conversation

Heenawter commented Jun 12, 2023 • edited Loading

Summary

Videos

Checklist

For maintainers

Heenawter Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Heenawter Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Heenawter Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Heenawter Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 14, 2023

nreese Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

nreese Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 19, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

andreadelrio left a comment

Choose a reason for hiding this comment

Heenawter commented Jun 12, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

Heenawter Jun 14, 2023 •

edited

Loading

nreese Jun 15, 2023 •

edited

Loading

nreese Jun 15, 2023 •

edited

Loading

nreese left a comment •

edited

Loading

Heenawter Jun 15, 2023 •

edited

Loading

Heenawter Jun 15, 2023 •

edited

Loading