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

[Embeddable Rebuild] [Controls] Add drag and drop to control group #188687

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Jul 18, 2024

Summary

Note

This PR has no user-facing changes - minus one small style change (which is a small selector simplification and doesn't actually change anything), all work is contained in the examples plugin.

This PR adds drag and drop to the refactored control group in the examples plugin.

Jul-18-2024 16-24-32

Checklist

For maintainers

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Project:Controls project:embeddableRebuild labels Jul 18, 2024
@Heenawter Heenawter self-assigned this Jul 18, 2024
@Heenawter Heenawter force-pushed the emeddableRebuild_controls_drag-n-drop_2024-07-17 branch from 075c372 to 638a33e Compare July 18, 2024 17:56
@Heenawter
Copy link
Contributor Author

/ci

@@ -85,7 +88,7 @@ export function initControlsManager(initialControlPanelsState: ControlPanelsStat
}

return {
controlsInOrder$: controlsInOrder$ as PublishingSubject<Array<{ id: string; type: string }>>,
controlsInOrder$,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to make this a public BehaviorSubject rather than adding a setter for the control order - this felt cleaner to me.

@@ -185,7 +185,7 @@ export const getTimesliderControlFactory = (
const viewModeSubject =
getViewModeSubject(controlGroupApi) ?? new BehaviorSubject('view' as ViewMode);

const defaultControl = initializeDefaultControlApi(initialState);
const defaultControl = initializeDefaultControlApi({ ...initialState, width: 'large' });
Copy link
Contributor Author

@Heenawter Heenawter Jul 18, 2024

Choose a reason for hiding this comment

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

The time slider should always have a width of large - this value cannot be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you turn your comment into a comment in the code please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - done in 34ac226

.controlFrame__formControlLayout {
opacity: 0; // hide dragged control, while control is dragged its replaced with ControlClone component
}
opacity: 0; // hide dragged control, while control is dragged its replaced with ControlClone component
Copy link
Contributor Author

@Heenawter Heenawter Jul 18, 2024

Choose a reason for hiding this comment

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

I tested this on both the new control group (in the examples plugin) and the old one (in the controls plugin) - the more generic selector works in both cases. No need to target euiFormRow__labelWrapper and controlFrame__formControlLayout explicitly to hide the control when dragging.

Controls on Dashboard after this change:

Jul-18-2024 16-29-40

Notice that, with this change, the control is still hidden on drag as expected

@Heenawter
Copy link
Contributor Author

/ci

@Heenawter
Copy link
Contributor Author

/ci

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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 202.2KB -182.0B

History

cc @Heenawter

@Heenawter Heenawter marked this pull request as ready for review July 18, 2024 23:13
@Heenawter Heenawter requested review from a team as code owners July 18, 2024 23:13
@elasticmachine
Copy link
Contributor

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

@Heenawter Heenawter requested a review from nreese July 19, 2024 17:16
@@ -186,3 +212,49 @@ export const ControlPanel = <ApiType extends DefaultControlApi = DefaultControlA
</EuiFlexItem>
);
};

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

How about putting ControlClone into its own file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1d0b5d2

@@ -102,6 +124,8 @@ export const ControlPanel = <ApiType extends DefaultControlApi = DefaultControlA

return (
<EuiFlexItem
ref={setNodeRef}
style={style}
grow={grow}
data-control-id={api?.uuid}
Copy link
Contributor

Choose a reason for hiding this comment

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

uuid is now passed in. How about using uuid instead of api?.uuid? What is data-control-id attribute used for?

Copy link
Contributor Author

@Heenawter Heenawter Jul 22, 2024

Choose a reason for hiding this comment

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

We use data-control-id for pretty much all of our control functional tests (via the controls page object) - not sure why we chose to use this attribute instead of just data-test-subj but this is legacy so not sure we want to change it. Replaced api.uuid with uuid in 22de169 though 👍

@@ -185,7 +185,7 @@ export const getTimesliderControlFactory = (
const viewModeSubject =
getViewModeSubject(controlGroupApi) ?? new BehaviorSubject('view' as ViewMode);

const defaultControl = initializeDefaultControlApi(initialState);
const defaultControl = initializeDefaultControlApi({ ...initialState, width: 'large' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you turn your comment into a comment in the code please

Copy link
Contributor

@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, one step closer to being feature complete
code review, tested in chrome

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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 202.2KB -182.0B

History

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

cc @Heenawter

@Heenawter Heenawter merged commit fa0ef37 into elastic:main Jul 22, 2024
19 checks passed
@Heenawter Heenawter deleted the emeddableRebuild_controls_drag-n-drop_2024-07-17 branch July 22, 2024 20:00
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 22, 2024
nreese added a commit that referenced this pull request Jul 29, 2024
While reviewing #188687, I noticed
that controls where still getting filtered by timeslider changes even
when chaining was disabled or the control was to the left of the
timeslider. This PR resolves this issue by only passing in timeslice
from `chaining$` instead of `controlGroupFetch$`.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:Controls project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants