Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add openAddDataControlFlyout to API #42

Conversation

Heenawter
Copy link

@Heenawter Heenawter commented Aug 8, 2024

This migrates some of my work from elastic#190185 into a separate PR

@@ -66,7 +69,9 @@ export type ControlGroupApi = PresentationContainer &
ignoreParentSettings$: PublishingSubject<ParentIgnoreSettings | undefined>;
allowExpensiveQueries$: PublishingSubject<boolean>;
untilInitialized: () => Promise<void>;
openAddDataControlFlyout: () => void;
openAddDataControlFlyout: (settings?: {
Copy link
Owner

@nreese nreese Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since settings is just a wrapper around controlInputTransform? How about just openAddDataControlFlyout: (controlInputTransform?: ControlInputTransform) => void

Copy link
Author

@Heenawter Heenawter Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There used to be more than one setting for the legacy version of openAddDataControlFlyout (it included an onSave callback) - I still haven't fully narrowed down which ones are necessary, and I won't know until the control group renderer is fully converted. For now, maybe we keep that extra level?

panelType: controlType,
initialState,
initialState: controlInputTransform!(
initialState as Partial<ControlGroupSerializedState>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be Partial<DefaultControlState> since the initialState is the control's initial state?

Copy link
Author

@Heenawter Heenawter Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a bad cast here, woops! Technically it should be Partial<ControlInput> because of how ControlInputTransform is defined in src/plugins/controls/common/types.ts. But since we don't want to touch any of the stuff that is shared between React controls and legacy at this point, perhaps we just cast to DataControlEditorState? This satisfies Partial<ControlInput>

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. We can always iterator on this

Copy link
Owner

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
code review only

@Heenawter Heenawter marked this pull request as ready for review August 8, 2024 22:04
@nreese nreese merged commit 6582cf5 into nreese:move_react_controls_into_controls_plugin Aug 8, 2024
1 of 2 checks passed
@Heenawter Heenawter deleted the embeddableRebuild_add-panel-flyout_2024-08-08 branch August 8, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants