[Controls] Add "Apply" button by Heenawter · Pull Request #174714 · elastic/kibana

Heenawter · 2024-01-11T17:53:26Z

Summary

This PR adds the option to stop selections from being auto-applied - instead, authors can make it so that their selections are only applied once the new apply button is clicked:

Screen.Recording.2024-03-04.at.4.39.15.PM.mov

Brief Summary of Changes

Publishing Filters
We used to publish the control group filters as soon as any child changed its output - however, if the apply button is enabled, this logic no longer works. So, we added an extra step to the publishing of filters:
1. When a child's output changes, we check if the apply button is enabled
2. If it is disabled, we publish the filters to the control group output right away (like we used to); otherwise, we push the new filters to the unpublishedFilters array.
3. Clicking the apply button will publish the unpublishedFilters array.
Unsaved Changes
We used to publish control group unsaved changes whenever anything about the children panels' persistable input changed - however, this no longer works with the apply button because we don't want selections to trigger unsaved changes when it is enabled.

To get around this, we no longer take into account selections when checking for unsaved changes - instead, we compare the output filters from the control group to determine whether anything about the children changed. As described above, if the control group has "auto-apply selections" turned off, the control group waits to change its output until the apply button is clicked - therefore, unsaved changes will also not be triggered until the apply button is clicked. This unsaved changes logic works regardless of if the apply button is enabled or not.
Saved Object
This required changes to the dashboard saved object because of how we store the control group input as part of the dashboard SO - so, we are now on a second version for the Dashboard SO. I've also made this version slightly less strict by allowing unknown attributes in the schema - that way, unknown attributes will no longer throw an error (as they do on version 1).

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
Flaky Test Runner was used on any tests changed
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

… src/core/server/integration_tests/ci_checks'

nickpeihl

Adding a few questions while I continue reviewing.

test/api_integration/apis/saved_objects/bulk_get.ts

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

nickpeihl · 2024-03-06T21:55:56Z

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

+                    <EuiFlexItem grow={false}>
+                      <EuiButtonIcon
+                        size="m"
+                        disabled={!applyButtonEnabled}
+                        iconSize="m"
+                        display="fill"
+                        color={'success'}
+                        iconType={'check'}
+                        data-test-subj="controlGroup--applyFiltersButton"
+                        aria-label={ControlGroupStrings.management.getApplyButtonTitle(
+                          applyButtonEnabled
+                        )}
+                        onClick={() => {
+                          if (unpublishedFilters) controlGroup.publishFilters(unpublishedFilters);
+                        }}
+                      />
+                    </EuiFlexItem>


While I understand the issue with the tooltip bug, it's still not a great experience IMO. The Refresh button above shows a tooltip on hover so I kinda wish this button also showed the tooltip. That said, this is just a small annoyance to me and probably not worth holding up the PR.

apply-button.mp4

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

nickpeihl · 2024-03-07T14:25:41Z

src/plugins/controls/public/time_slider/components/time_slider_strings.ts

+      }),
+    getPlayButtonDisabledTooltip: () =>
+      i18n.translate('controls.timeSlider.playButtonTooltip.disabled', {
+        defaultMessage: 'Only applicable when auto-apply selections is enabled.',


This should probably align with the copy in the Control Settings. Maybe something like "Apply selections automatically" is disabled in Control Settings.

cc @amyjtechwriter

I'm not convinced we even need this tooltip, as described in #174714 (comment).... Interested in what @andreadelrio and @amyjtechwriter think.

Text changed in 88f418e

cc @amyjtechwriter

Are we allowed to remove the tooltip completely still? I'd vote for that option.

@amyjtechwriter We've decided to keep the tooltip only in edit mode - so, with that in mind, does the copy sound okay to you?

nickpeihl · 2024-03-07T21:14:39Z

src/plugins/dashboard/server/dashboard_saved_object/dashboard_saved_object.ts

          controlStyle: { type: 'keyword', index: false, doc_values: false },
          chainingSystem: { type: 'keyword', index: false, doc_values: false },
          panelsJSON: { type: 'text', index: false },
+          showApplySelections: { type: 'boolean', index: false },


I think (@jloleysens please keep me honest here 😅) ideally the dashboard.controlGroupInput mapping in packages/kbn-check-mappings-update-cli/current_mappings.json would be similar to the lens.state mapping. The data is still stored in _source but not in separate fields. However, we might need multiple modelVersions to achieve that.

e.g.

... "dashboard": { "properties": { "controlGroupInput": { "dynamic": false, "properties": {} }, ...

But I wonder if we can keep the index: false, doc_values: false settings to unblock this PR and remove those fields in a separate PR later?

Add tests for dashboard model version

…fix'

andreadelrio

Left you a comment about the disabled state.

…r/kibana into controls-apply-button_2023-01-3

Heenawter · 2024-03-08T20:18:12Z

@andreadelrio Addressed in #174714 (comment) and #174714 (comment) 🙇

src/plugins/dashboard/common/content_management/v1/types.ts

src/plugins/dashboard/server/dashboard_saved_object/dashboard_saved_object.ts

nickpeihl

lgtm! thanks for getting us closer to parity on input controls!

code review and tested controls and time slider controls with and without apply button

andreadelrio

🚀 Design changes LGTM. Fantastic job @Heenawter! Exciting to see our new controls even closer to the legacy ones.

kibana-ci · 2024-03-11T19:09:37Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 9fbe3f4

Failed CI Steps

FTR Configs #55

Test Failures

[job] [logs] FTR Configs #55 / SLO API Tests Delete SLOs deletes new slo saved object and transforms

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	430	429	-1

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`	324	332	+8

Async chunks

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

id	before	after	diff
`controls`	202.3KB	201.9KB	-324.0B
`dashboard`	383.9KB	383.9KB	-43.0B
`securitySolution`	12.8MB	12.8MB	-2.0B
total			-369.0B

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`	16	20	+4

Page load bundle

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

id	before	after	diff
`controls`	42.1KB	42.5KB	+328.0B
`dashboard`	37.3KB	37.4KB	+110.0B
`presentationUtil`	36.2KB	36.3KB	+77.0B
total			+515.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	332	340	+8

History

💛 Build #196913 was flaky 6a126cb
💚 Build #196742 succeeded bbc8dab
💔 Build #196741 failed b686518
💔 Build #196721 failed 0d58cea
💛 Build #196691 was flaky ec16487
💛 Build #196524 was flaky 4441d11

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

cc @Heenawter

Heenawter added release_note:enhancement enhancement New value added to drive a business result Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// backport:skip This PR does not require backporting labels Jan 11, 2024

Heenawter self-assigned this Jan 11, 2024

Heenawter force-pushed the controls-apply-button_2023-01-3 branch 2 times, most recently from c24a5e0 to 80d9080 Compare January 15, 2024 20:46

Heenawter force-pushed the controls-apply-button_2023-01-3 branch 2 times, most recently from b509f77 to db8aa09 Compare January 31, 2024 16:23

Heenawter added 16 commits February 5, 2024 11:01

First design draft

d2db0d8

First attempt at SO + CM versioning

30d519c

Clean up CM + SO stuff

269fb86

Reset selections - attempt 1

f1e7878

Remove unnecessary down transform

e6b0df9

Undo V1 changes

af9f389

First attempt at apply changes button

4c1f3b1

Diff output filters to determine apply button state

fff3c8a

Cleaner implementation

f1bd420

Fix range slider filter output

e67ab45

Disable time slider button when auto-apply false

5ea9324

Trigger filter changes when settings change

6ddeb92

Start some cleanup work

4785379

Clean up the styling

a63ddd1

Fix conflicts + default persistable input

f4baa90

Another attempt at apply button

c2cc71f

Heenawter force-pushed the controls-apply-button_2023-01-3 branch from 32536c4 to c2cc71f Compare February 5, 2024 18:03

Heenawter added 4 commits February 5, 2024 17:13

First attempt at making unsaved changes respond properly

9da9021

Remove reset button

de7589b

Do some cleanup

4f384c8

More cleanup + i18n

060749e

Heenawter and others added 3 commits March 7, 2024 11:58

Update mappings v2

3a4101b

Quick fix

f3c27dc

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

0a230e1

… src/core/server/integration_tests/ci_checks'

nickpeihl reviewed Mar 7, 2024

View reviewed changes

Heenawter and others added 4 commits March 7, 2024 14:48

Remove special showApplySelections logic from comparison

4441d11

Add tests for dashboard model version

d4a39d7

Merge pull request #8 from nickpeihl/dashboard-mv2-tests

0ef5e41

Add tests for dashboard model version

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

ec16487

…fix'

andreadelrio requested changes Mar 8, 2024

View reviewed changes

Heenawter added 3 commits March 8, 2024 12:49

Fix time slider play button

5aca279

Merge branch 'controls-apply-button_2023-01-3' of github.com:heenawte…

88f418e

…r/kibana into controls-apply-button_2023-01-3

Add tooltip to disabled apply button

cf9cb52

Merge branch 'main' into controls-apply-button_2023-01-3

0d58cea

nickpeihl reviewed Mar 8, 2024

View reviewed changes

src/plugins/dashboard/common/content_management/v1/types.ts Show resolved Hide resolved

nickpeihl reviewed Mar 8, 2024

View reviewed changes

src/plugins/dashboard/server/dashboard_saved_object/dashboard_saved_object.ts Show resolved Hide resolved

nickpeihl approved these changes Mar 8, 2024

View reviewed changes

andreadelrio self-requested a review March 8, 2024 22:00

andreadelrio approved these changes Mar 8, 2024

View reviewed changes

Heenawter and others added 4 commits March 8, 2024 15:31

Refetch apply button in tests due to useMemo

b686518

Clean up imports

bbc8dab

Merge branch 'main' into controls-apply-button_2023-01-3

6a126cb

Change copy for apply setting

9fbe3f4

Heenawter mentioned this pull request Mar 11, 2024

[Dashboard] Clean up saved object mappings #178425

Closed

dimadavid approved these changes Mar 12, 2024

View reviewed changes

Heenawter merged commit 235c4d5 into elastic:main Mar 12, 2024

Heenawter deleted the controls-apply-button_2023-01-3 branch March 12, 2024 18:22

Heenawter mentioned this pull request May 3, 2024

[Controls][Performance] Do not immediately refresh after user select in combo-box #182543

Closed

Heenawter mentioned this pull request Aug 13, 2025

[Controls] Remove apply button in favour of Unified Search refresh #231488

Closed

Conversation

Heenawter commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Brief Summary of Changes

Checklist

For maintainers

Uh oh!

nickpeihl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickpeihl Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickpeihl Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

Heenawter Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Heenawter Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amyjtechwriter Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

Heenawter Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickpeihl Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

andreadelrio left a comment

Choose a reason for hiding this comment

Uh oh!

Heenawter commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickpeihl left a comment

Choose a reason for hiding this comment

Uh oh!

andreadelrio left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 11, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Heenawter commented Jan 11, 2024 •

edited

Loading

Heenawter Mar 7, 2024 •

edited

Loading

Heenawter Mar 8, 2024 •

edited

Loading

Heenawter Mar 11, 2024 •

edited

Loading

Heenawter commented Mar 8, 2024 •

edited

Loading