[control group] apply selections on reset #189830
Conversation
|
/ci |
|
/ci |
|
@elasticmachine merge upstream |
|
/ci |
|
/ci |
| @@ -196,7 +196,6 @@ export const OptionsListPopoverSuggestions = ({ | |||
| )} | |||
| emptyMessage={<OptionsListPopoverEmptyMessage showOnlySelected={showOnlySelected} />} | |||
| onChange={(newSuggestions, _, changedOption) => { | |||
| setSelectableOptions(newSuggestions); | |||
There was a problem hiding this comment.
Removed because its not needed. setSelectableOptions when useEffect runs because of observable changes made when makeSelection fires
nreese
added
Feature:Input Control
|
@elasticmachine merge upstream |
| if (existsSelected) selections.setExistsSelected(false); | ||
| selections.setSelectedOptions( | ||
| selectedOptions ? [...selectedOptions, keyAsType] : [keyAsType] | ||
| ); |
There was a problem hiding this comment.
As a cleanup, I wonder if we could move the makeSelection function into initializeOptionsListSelections? Not a blocker, though
There was a problem hiding this comment.
The problem with moving makeSelection into initializeOptionsListSelections is that in initializeOptionsListSelections the subjects are BehaviorSubject instead of PublishingSubjects. This makes it easy to slip up and call subject.next instead of the setter. Is it better to co-locate this code in initializeOptionsListSelections or have type protections?
There was a problem hiding this comment.
Ah, that's a fair point - I'm fine leaving it where it is for now for type safety in that case 👍
| await Promise.all(filtersReadyPromises); | ||
| applySelections(); |
There was a problem hiding this comment.
We should probably only call applySelections if auto-apply is disabled - otherwise, we are publishing twice when it is enabled due to the unpublishedFilters$ subscription in the control group selections manager.
There was a problem hiding this comment.
I added an if statement around applySelections. applySelections will not publish filters if there are no changes. See the below implementation. So I am not sure if the if statement is needed. Let me know if I should take it out.
function applySelections() {
if (!deepEqual(filters$.value, unpublishedFilters$.value)) {
filters$.next(unpublishedFilters$.value);
}
if (!deepEqual(timeslice$.value, unpublishedTimeslice$.value)) {
timeslice$.next(unpublishedTimeslice$.value);
}
}
There was a problem hiding this comment.
Ah, good point - I personally still think we should keep the applySelections in an if (there is more overhead to calling applySelections twice than there is to check the value of applySelections). But ultimately I'm fine either way, since the at least the filters$ subject won't emit twice 👍
examples/controls_example/public/app/react_control_example/react_control_example.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
I think whatever is causing #190066 is also impacting controls here - which makes me thinks it might be related to #189128. To see this, add a new control and try to click save - unsaved changes gets stuck:
Screen.Recording.2024-08-07.at.4.03.33.PM.mov
This is not related to this work, so it is not a blocker - but it's definitely something we will want to resolve before the control group can be replaced 👍 Code review + tested locally and everything related to applying the selections on reset LGTM 🎉
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Fixes #189580
PR awaits until all control filters are ready and then applies selections during reset.