[Controls] Clear range/time slider selections when field changes #129824

Heenawter · 2022-04-07T22:28:27Z

Summary

Before this change, old selections would remain when a control's field was edited for both range and time slider controls - this would, more often than not, result in an invalid selection for the new control. Now, if the field is different when the control editor is saved, the previous selections are cleared for all three control types.

How to test

For both range and time slider controls, you should:

Test that changing the field clears selections:
1. Create a control of that specific type
2. Make some sort of selection with that control
3. Edit the control and change the field that is being referenced
4. Save your new control
5. Notice that the old selections are gone
Test that only field changes clear selections:
1. Create a control of that specific type
2. Make some sort of selection with that control
3. Edit the control and change the size and/or the label and/or some other setting that is not the field
4. Save your new control
5. Notice that the old selections remain

Videos

Changing field clears selection:

2022-04-19_RangeFieldClearOldSelctions.mp4

Changing anything else does not clear selection:

2022-04-19_RangeKeepOldSelctions.mp4.mp4

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2022-04-18T21:15:36Z

src/plugins/controls/public/control_types/range_slider/range_slider.component.tsx

@@ -45,16 +45,19 @@ export const RangeSliderComponent: FC<Props> = ({ componentStateSubject }) => {
    componentStateSubject.getValue()
  );

-  const { value = ['', ''], id, title } = useEmbeddableSelector((state) => state);
+  const { value, id, title } = useEmbeddableSelector((state) => state);


When it was previously const { value = ['', ''], id, title }, this would cause infinite recursion because of the new useEffect that depends on the value changing - therefore, to avoid this, I simply do this check as part of the new useEffect where I set the selected value (line 53)

Heenawter · 2022-04-18T21:19:28Z

src/plugins/controls/public/control_types/range_slider/range_slider.component.tsx


  const [selectedValue, setSelectedValue] = useState<RangeValue>(value || ['', '']);

+  useEffect(() => {


In order to updated the selectedValue when the state value changes via the resaveTransformFunction, I had to split the previous useCallback in to two steps rather than one - the onChangeComplete callback now simply dispatches the new value via selectRange, and the useEffect listens for changes to value in order to force selectedValue to update alongside value.

Note that, for the options list, this was already handled by the useEffect on line 75 of options_list_component.tsx because we don't have a separate useState for which values are selected - instead, we handle changes directly from when the state changes,

Using a UseEffect and a useState here seems a bit overcomplicated for the use case. Maybe we should double check with @cqliu1 when she's back, but I was able to get the same behaviour by removing the useState and useEffect here entirely and just using the value directly from the embeddable input instead of mirroring it:

<RangeSliderPopover ... other stuff value={value ?? ['', '']} />

Because the slider itself can't handle null / undefined, notice how I've initialized the default value.

Ahhh, that's totally fair. I implemented these changes for both the time and range sliders in 67d6f12 - as long as @cqliu1 and @crob611 don't see any unintended consequences with this, we'll go with it! I tested it locally and everything worked as expected, but it's good to be extra careful 👍

Awesome, thanks for implementing this. And yeah it'll be good to check with them because I could've potentially not covered an edge case with my testing of it. It seems fine, but they might know better!

Heenawter · 2022-04-19T20:16:05Z

src/plugins/controls/common/types.ts

+export type DataControlInput = ControlInput & {
+  fieldName: string;
+  dataViewId: string;
+};


Currently, all of the controls we have share fieldName and dataViewId.

While this is true for now, in the future we may have different types of controls - for example, those that set dashboard variables or ones that change configuration options - and these new controls may not need all three of these properties. Therefore, rather than simply making these two properties part of the generic ControlInput, I defined a separate type for data-specific controls DataControlInput that all of our current controls should use instead of the generic ControlInput

Thanks again for tackling this!

Heenawter · 2022-04-19T20:50:12Z

src/plugins/controls/public/control_types/options_list/options_list_editor.tsx

@@ -60,7 +60,7 @@ export const OptionsListEditor = ({
        initialInput?.dataViewId ?? getRelevantDataViewId?.() ?? (await getDefaultId());
      let dataView: DataView | undefined;
      if (initialId) {
-        onChange({ dataViewId: initialId });
+        onChange({ dataViewId: initialId, fieldName: initialInput?.fieldName });


Because we weren't making the fieldName part of the initial input on mount for editing existing controls, this meant it was originally undefined - so if you changed, for example, just the control label (and did not specifically re-click the exact same field), then the selections would be unnecessarily cleared because the original field would not equal undefined and therefore diffDataFetchProps would be called. This happened for all control types.

By making fieldName part of the initial input (when editing an existing control), this is prevented 👍

I undid this change, and was unable to reproduce the issue where changing the label and not the fieldName would clear the selections on an options list control. I believe this is because the Options List Presave transform does a null check as well as the deepEqual.

For an options list, the logic looks like:
newInput.fieldName && !deepEqual(newInput.fieldName, embeddable.getInput().fieldName),

Whereas in the Range slider and Time Slider the logic looks like
!deepEqual(newInput.fieldName, embeddable.getInput().fieldName)

I think it's a bit simpler, and more easily maintainable if this is fixed from within the presaveTransformFunction.

That's a great point! I implemented the change in presaveTransformFunction for each control instead, and it works great :)

elasticmachine · 2022-04-20T15:01:58Z

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

ThomThomson

I very much liked the test additions, and the new interface and it's description / explanation are spot on, so great job!

I left a couple comments about code architecture / DX, but this LGTM!

ThomThomson · 2022-04-20T20:41:06Z

src/plugins/controls/common/types.ts

+export type DataControlInput = ControlInput & {
+  fieldName: string;
+  dataViewId: string;
+};


Thanks again for tackling this!

ThomThomson · 2022-04-20T21:33:47Z

src/plugins/controls/public/control_types/options_list/options_list_editor.tsx

@@ -60,7 +60,7 @@ export const OptionsListEditor = ({
        initialInput?.dataViewId ?? getRelevantDataViewId?.() ?? (await getDefaultId());
      let dataView: DataView | undefined;
      if (initialId) {
-        onChange({ dataViewId: initialId });
+        onChange({ dataViewId: initialId, fieldName: initialInput?.fieldName });


I undid this change, and was unable to reproduce the issue where changing the label and not the fieldName would clear the selections on an options list control. I believe this is because the Options List Presave transform does a null check as well as the deepEqual.

For an options list, the logic looks like:
newInput.fieldName && !deepEqual(newInput.fieldName, embeddable.getInput().fieldName),

Whereas in the Range slider and Time Slider the logic looks like
!deepEqual(newInput.fieldName, embeddable.getInput().fieldName)

I think it's a bit simpler, and more easily maintainable if this is fixed from within the presaveTransformFunction.

ThomThomson · 2022-04-20T21:44:19Z

src/plugins/controls/public/control_types/range_slider/range_slider.component.tsx


  const [selectedValue, setSelectedValue] = useState<RangeValue>(value || ['', '']);

+  useEffect(() => {


Using a UseEffect and a useState here seems a bit overcomplicated for the use case. Maybe we should double check with @cqliu1 when she's back, but I was able to get the same behaviour by removing the useState and useEffect here entirely and just using the value directly from the embeddable input instead of mirroring it:

<RangeSliderPopover ... other stuff value={value ?? ['', '']} />

Because the slider itself can't handle null / undefined, notice how I've initialized the default value.

kibana-ci · 2022-04-20T23:58:07Z

💚 Build Succeeded

Metrics [docs]

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`	182	174	-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`	437.6KB	437.6KB	-64.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`	4	5	+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`	32.4KB	32.4KB	+60.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	188	180	-8

History

💔 Build #39644 failed 3839fbaa77ec2a436a2e5f505f9211121e71dee9
💚 Build #39612 succeeded bb3d368
💚 Build #39274 succeeded 9ba67b6
💛 Build #39265 was flaky c3c2450
💔 Build #39229 failed a7d91af4ddf1a63c2f5f40ea42ee2b9675c7aa22
💔 Build #38871 failed dce4354353b3cb3d568ee5425395c934c3c2ef5d

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

cc @Heenawter

kibanamachine · 2022-04-21T19:42:10Z

💔 All backports failed

Status	Branch	Result
❌	8.2	Backport failed because of merge conflicts
❌	8.1	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 129824

Questions ?

Please refer to the Backport tool documentation

…stic#129824) * Reset selections on save of existing control * Allow reset to force render for range/time sliders * Reset selections only when field name or data view changes * Make generic DataControlInput interface * Fix infinite useEffect + imports + types * Simpler solution without resetSelections() * Add functional tests * Apply Devon's changes (cherry picked from commit 5a86421) # Conflicts: # src/plugins/controls/common/control_types/options_list/types.ts

…9824) (#130827) * Reset selections on save of existing control * Allow reset to force render for range/time sliders * Reset selections only when field name or data view changes * Make generic DataControlInput interface * Fix infinite useEffect + imports + types * Simpler solution without resetSelections() * Add functional tests * Apply Devon's changes (cherry picked from commit 5a86421) # Conflicts: # src/plugins/controls/common/control_types/options_list/types.ts

…stic#129824) * Reset selections on save of existing control * Allow reset to force render for range/time sliders * Reset selections only when field name or data view changes * Make generic DataControlInput interface * Fix infinite useEffect + imports + types * Simpler solution without resetSelections() * Add functional tests * Apply Devon's changes

Heenawter self-assigned this Apr 7, 2022

Heenawter added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 7, 2022

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch 2 times, most recently from 047472d to f223cba Compare April 14, 2022 16:36

Heenawter added 3 commits April 18, 2022 13:44

Reset selections on save of existing control

7f04347

Allow reset to force render for range/time sliders

3696093

Reset selections only when field name or data view changes

0dd5ee0

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch 2 times, most recently from f6c94a4 to a249f68 Compare April 18, 2022 20:32

Make generic DataControlInput interface

a8bbfcf

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch from a249f68 to 1271968 Compare April 18, 2022 21:09

Heenawter commented Apr 18, 2022

View reviewed changes

Fix infinite useEffect + imports + types

422dd50

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch from a7d91af to c18e284 Compare April 19, 2022 20:15

Heenawter commented Apr 19, 2022

View reviewed changes

Simpler solution without resetSelections()

c3c2450

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch from c18e284 to c3c2450 Compare April 19, 2022 21:27

Heenawter changed the title ~~[Controls] Clear control selections when field and/or data view changes~~ [Controls] Clear range/time slider selections when control' field changes Apr 19, 2022

Heenawter changed the title ~~[Controls] Clear range/time slider selections when control' field changes~~ [Controls] Clear range/time slider selections when field changes Apr 19, 2022

Add functional tests

9ba67b6

Heenawter marked this pull request as ready for review April 20, 2022 15:01

Heenawter requested a review from a team as a code owner April 20, 2022 15:01

Heenawter added v8.2.1 v8.1.4 labels Apr 20, 2022

Merge branch 'main' into clear-selections-on-change_2022-04-06

bb3d368

ThomThomson approved these changes Apr 20, 2022

View reviewed changes

Heenawter force-pushed the clear-selections-on-change_2022-04-06 branch 2 times, most recently from 3839fba to 67d6f12 Compare April 20, 2022 22:38

Apply Devon's changes

67d6f12

Heenawter merged commit 5a86421 into elastic:main Apr 21, 2022

Heenawter mentioned this pull request Apr 21, 2022

[8.2] [Controls] Clear range/time slider selections when field changes (#129824) #130827

Merged

Heenawter removed v8.1.3 v8.1.4 labels Apr 21, 2022

Heenawter deleted the clear-selections-on-change_2022-04-06 branch April 21, 2022 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Controls] Clear range/time slider selections when field changes #129824

[Controls] Clear range/time slider selections when field changes #129824

Heenawter commented Apr 7, 2022 •

edited

Loading

Heenawter Apr 18, 2022 •

edited

Loading

Heenawter Apr 18, 2022 •

edited

Loading

ThomThomson Apr 20, 2022

Heenawter Apr 20, 2022 •

edited

Loading

ThomThomson Apr 21, 2022

Heenawter Apr 19, 2022 •

edited

Loading

ThomThomson Apr 20, 2022

Heenawter Apr 19, 2022 •

edited

Loading

ThomThomson Apr 20, 2022

Heenawter Apr 20, 2022

elasticmachine commented Apr 20, 2022

ThomThomson left a comment

ThomThomson Apr 20, 2022

ThomThomson Apr 20, 2022

ThomThomson Apr 20, 2022

kibana-ci commented Apr 20, 2022

API count

kibanamachine commented Apr 21, 2022


		const [selectedValue, setSelectedValue] = useState<RangeValue>(value \|\| ['', '']);

		useEffect(() => {

[Controls] Clear range/time slider selections when field changes #129824

[Controls] Clear range/time slider selections when field changes #129824

Conversation

Heenawter commented Apr 7, 2022 • edited Loading

Summary

How to test

Videos

Checklist

For maintainers

Heenawter Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Heenawter Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heenawter Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 20, 2022

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

kibana-ci commented Apr 20, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

kibanamachine commented Apr 21, 2022

💔 All backports failed

Manual backport

Questions ?

Heenawter commented Apr 7, 2022 •

edited

Loading

Heenawter Apr 18, 2022 •

edited

Loading

Heenawter Apr 18, 2022 •

edited

Loading

Heenawter Apr 20, 2022 •

edited

Loading

Heenawter Apr 19, 2022 •

edited

Loading

Heenawter Apr 19, 2022 •

edited

Loading