[Dashboard] [Controls] Use `uiActions` for control hover actions #153065

Heenawter · 2023-03-09T21:55:57Z

Closes #143585
Closes #151767
Closes #152609

Summary

This PR accomplishes three things, the first of which is moving the edit/delete control hover actions to use the uiActions service - this is the first step in moving existing panel actions (such as replacing the panel, opening the panel settings flyout, etc.) to this hover framework, which is outlined in this issue.

While this was the primary goal of this PR, this also made the following fixes possible:

Since I was refactoring the control editor flyout code as part of this PR, I made it so that changes to the control's width/grow properties are only applied when the changes are saved rather than being automatically applied.

Before After
Since the edit control button is no longer a custom component, the tooltip now responds to focus as expected.

Before After

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 was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-03-14T21:04:46Z

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

@@ -106,7 +106,7 @@ const SortableControlInner = forwardRef<
        style={style}
      >
        <ControlFrame
-          enableActions={isEditable && draggingIndex === -1}
+          enableActions={draggingIndex === -1}


The available actions are now determined based on their individual compatibility + the disabled actions from the control group - so, we don't need to know if the dashboard is in edit mode for this anymore.

Heenawter · 2023-03-14T21:04:54Z

src/plugins/controls/public/options_list/embeddable/options_list_embeddable_factory.tsx

+      // if the field name or data view id has changed in this editing session, reset all selections
+      newInput.selectedOptions = undefined;
+      newInput.existsSelected = undefined;
+      newInput.exclude = undefined;
+      newInput.sort = undefined;


Before these were added, existsSelected, exclude, and sort were all remembered when the data view and/or field of a control was changed - while it doesn't cause any apparent issues (unlike selectedOptions, which was the only thing included before), it goes against expected behaviour IMO. If the data view and/or field of a control is changed, I would expect the entire state of the control to be reset.

Heenawter · 2023-03-14T21:05:12Z

src/plugins/presentation_util/public/components/floating_actions/floating_actions.scss

@@ -17,7 +17,7 @@
    .presentationUtil__floatingActions {
      opacity: 1;
      visibility: visible;
-      transition: visibility .1s, opacity .1s;
+      transition: visibility $euiAnimSpeedFast, opacity $euiAnimSpeedFast;


cc @andreadelrio We talked about this 👍

Heenawter · 2023-03-14T21:05:23Z

src/plugins/presentation_util/public/components/floating_actions/floating_actions.tsx

+  const viewMode = select((state) => state.explicitInput.viewMode);
+  const disabledActions = select((state) => state.explicitInput.disabledActions);


I tried to pull as much logic as possible into the FloatingActions component to keep the consumers as clean as possible - this meant adding the action fetching logic to this component. However, this caused an issue where I needed things to be re-fetched when certain control group state changed - for example, view mode or the disabled actions.

To get around this, I am taking in the Redux state selector hook so that the FloatingActions component can re-fetch the compatible actions as these parts of state change... I tried a few different things, including taking each piece of state as a prop instead, but this seemed the cleanest to me? It ties the FloatingActions component to the ReduxEmbeddableState though, so I'm open to other ideas 👀

Makes sense to me. Thanks for the explanation.

I think ideally we wouldn't want the floating actions tied to any sort of redux-specific state. Most embeddable containers won't use that I think. Is there any complexity in just passing in viewMode and disabledActions as props?

elasticmachine · 2023-03-15T14:43:14Z

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

Heenawter · 2023-03-15T18:26:21Z

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

+        // @ts-ignore - TODO: Remove this once https://github.com/elastic/eui/pull/6645 lands in Kibana
+        focusTrapProps: { scrollLock: true },


Refer to #153227

Just for clarification, am I correct that adding scrollLock: true in this PR does not help until #153227 is in main? Basically, we need scrollLock on the .kbnBody class, too?

nickpeihl

lgtm!

code review and tested in chrome.

AFAICT, the weird flyout rendering issue should be resolved in a future EUI update.

nickpeihl · 2023-03-20T16:20:17Z

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

+        // @ts-ignore - TODO: Remove this once https://github.com/elastic/eui/pull/6645 lands in Kibana
+        focusTrapProps: { scrollLock: true },


Just for clarification, am I correct that adding scrollLock: true in this PR does not help until #153227 is in main? Basically, we need scrollLock on the .kbnBody class, too?

nickpeihl · 2023-03-20T19:36:28Z

src/plugins/presentation_util/public/components/floating_actions/floating_actions.tsx

+  const viewMode = select((state) => state.explicitInput.viewMode);
+  const disabledActions = select((state) => state.explicitInput.disabledActions);


Makes sense to me. Thanks for the explanation.

Heenawter · 2023-03-20T19:40:47Z

@nickpeihl

Just for clarification, am I correct that adding scrollLock: true in this PR does not help until #153227 is in main? Basically, we need scrollLock on the .kbnBody class, too?

Yup, that's correct! And it should be in main now, so this should help reduce the frequency of the double animation we're seeing 🎉

ThomThomson

Looks like Nick beat me to it, but I figured I had enough content that it warranted a double-review. In short, everything looks good!

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

ThomThomson · 2023-03-20T19:05:23Z

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

+let reduxEmbeddablePackage: ReduxEmbeddablePackage;
+
+beforeAll(async () => {
+  reduxEmbeddablePackage = await lazyLoadReduxEmbeddablePackage();


Would it be quicker to use a mocked version of this? I am not entirely sure if one exists yet, but if not I will be adding it in #150121

It probably would be, yeah! We don't seem to have the mock yet, though - if you plan to add one, that would be awesome!

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

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

ThomThomson · 2023-03-20T19:16:56Z

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

+
+  const onCancel = () => {
+    if (
+      isEqual(panel.explicitInput, {


As a follow-up in some other PR maybe we should look into fixing the bug where the confirm model is shown if you create a new control then immediately cancel.

ThomThomson · 2023-03-20T19:53:21Z

src/plugins/presentation_util/public/components/floating_actions/floating_actions.tsx

+  const viewMode = select((state) => state.explicitInput.viewMode);
+  const disabledActions = select((state) => state.explicitInput.disabledActions);


I think ideally we wouldn't want the floating actions tied to any sort of redux-specific state. Most embeddable containers won't use that I think. Is there any complexity in just passing in viewMode and disabledActions as props?

Heenawter · 2023-03-20T20:57:18Z

I think ideally we wouldn't want the floating actions tied to any sort of redux-specific state. Most embeddable containers won't use that I think. Is there any complexity in just passing in viewMode and disabledActions as props?

@ThomThomson Nope - if passing through props is preferred, then we can totally do that 💃 I just wasn't sure which style would be preferred, so I opted for fewest-props - but I didn't necessarily feel super strong one way or another.

Moved to props in 505924e 👍

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

kibana-ci · 2023-03-20T22:04:28Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #10

Test Failures

[job] [logs] FTR Configs #10 / security APIs - Session Idle Session POST /internal/security/session should extend the session

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	150	153	+3
`presentationUtil`	154	155	+1
total			+4

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`	268	270	+2
`embeddable`	436	441	+5
total			+7

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`embeddable`	9	11	+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`	179.4KB	189.9KB	+10.5KB
`presentationUtil`	129.0KB	129.1KB	+2.0B
total			+10.5KB

Page load bundle

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

id	before	after	diff
`controls`	32.1KB	33.3KB	+1.1KB
`embeddable`	75.3KB	75.7KB	+404.0B
`presentationUtil`	35.2KB	36.0KB	+808.0B
total			+2.3KB

Unknown metric groups

API count

id	before	after	diff
`controls`	272	274	+2
`embeddable`	539	544	+5
total			+7

async chunk count

id	before	after	diff
`controls`	5	7	+2

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💚 Build #114046 succeeded e232839
💚 Build #113771 succeeded dfd67ac
💛 Build #113711 was flaky d40fa62
💚 Build #113699 succeeded f673e28e8701b57fe433d31514a8004b6a280a3f
💚 Build #113654 succeeded 6eac0e9f29ec8ca0eda1411c3755eb8f4241419c
💔 Build #113641 failed f21ca343402dbebc0e0b21517768a896b0f2587d

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

cc @Heenawter

andreadelrio

Design changes LGTM. Amazing work @Heenawter!

logeekal · 2023-04-04T13:25:45Z

Hello @Heenawter , @ThomThomson

One thought/suggestion came to me mind regarding the Actions. In addition to ACTION_EDIT_CONTROL, ACTION_DELETE_CONTROL, should we add DRAGGABLE because edit mode enabled these three things. We can make this the part of #154068.

This will help to make the control not draggable.

Let me know what you think.

Heenawter self-assigned this Mar 9, 2023

Heenawter force-pushed the use-actions-framework_2023-03-06 branch 11 times, most recently from 259531d to 344bc81 Compare March 10, 2023 23:37

Use EUI variables for hover animation

9dca4c7

Heenawter force-pushed the use-actions-framework_2023-03-06 branch 11 times, most recently from 9545168 to 9bfa2ac Compare March 14, 2023 20:56

Heenawter commented Mar 14, 2023

View reviewed changes

Heenawter marked this pull request as ready for review March 15, 2023 14:43

Heenawter requested review from a team as code owners March 15, 2023 14:43

Heenawter mentioned this pull request Mar 15, 2023

[EUI] Add scrollLock workaround CSS to Kibana's body #153227

Merged

4 tasks

Add scroll lock to edit control flyout

0c5f280

botelastic bot added the Feature:Embedding Embedding content via iFrame label Mar 15, 2023

Heenawter commented Mar 15, 2023

View reviewed changes

Heenawter added 2 commits March 15, 2023 12:26

Merge branch 'main' into use-actions-framework_2023-03-06

dfd67ac

Merge branch 'main' into use-actions-framework_2023-03-06

e232839

nickpeihl approved these changes Mar 20, 2023

View reviewed changes

ThomThomson approved these changes Mar 20, 2023

View reviewed changes

Address first round of comments

505924e

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

d442648

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

andreadelrio approved these changes Mar 20, 2023

View reviewed changes

Heenawter merged commit b6fb660 into elastic:main Mar 21, 2023

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Mar 21, 2023

Heenawter mentioned this pull request Mar 21, 2023

[Control Group]: Option to toggle floating actions #153019

Closed

Heenawter mentioned this pull request Jun 9, 2023

[Controls] Make "clear selections" a hover action for all data controls #159395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] [Controls] Use `uiActions` for control hover actions #153065

[Dashboard] [Controls] Use `uiActions` for control hover actions #153065

Heenawter commented Mar 9, 2023 •

edited

Loading

Heenawter Mar 14, 2023

Heenawter Mar 14, 2023

Heenawter Mar 14, 2023

Heenawter Mar 14, 2023

nickpeihl Mar 20, 2023

ThomThomson Mar 20, 2023

elasticmachine commented Mar 15, 2023

Heenawter Mar 15, 2023

nickpeihl Mar 20, 2023

nickpeihl left a comment

nickpeihl Mar 20, 2023

nickpeihl Mar 20, 2023

Heenawter commented Mar 20, 2023

ThomThomson left a comment

ThomThomson Mar 20, 2023

Heenawter Mar 20, 2023

ThomThomson Mar 20, 2023

ThomThomson Mar 20, 2023

Heenawter commented Mar 20, 2023

kibana-ci commented Mar 20, 2023

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

andreadelrio left a comment

logeekal commented Apr 4, 2023 •

edited

Loading

		const viewMode = select((state) => state.explicitInput.viewMode);
		const disabledActions = select((state) => state.explicitInput.disabledActions);

		// @ts-ignore - TODO: Remove this once https://github.com/elastic/eui/pull/6645 lands in Kibana
		focusTrapProps: { scrollLock: true },

[Dashboard] [Controls] Use uiActions for control hover actions #153065

[Dashboard] [Controls] Use uiActions for control hover actions #153065

Conversation

Heenawter commented Mar 9, 2023 • edited Loading

Summary

Checklist

For maintainers

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickpeihl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter commented Mar 20, 2023

ThomThomson left a comment

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

Choose a reason for hiding this comment

Heenawter commented Mar 20, 2023

kibana-ci commented Mar 20, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Any counts in public APIs

Async chunks

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

logeekal commented Apr 4, 2023 • edited Loading

[Dashboard] [Controls] Use `uiActions` for control hover actions #153065

[Dashboard] [Controls] Use `uiActions` for control hover actions #153065

Heenawter commented Mar 9, 2023 •

edited

Loading

logeekal commented Apr 4, 2023 •

edited

Loading