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

SceneCSSGridLayout: Add support for dragging and dropping panels #993

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kaydelaney
Copy link
Contributor

There's a lot of code here but the gist of it is enables support for dragging and dropping panels in the responsive grid layout (when paired with the PR linked below)

Let me know if there are any questions or parts of the code that could use further explanation.

Related: grafana/grafana#97495

@kaydelaney kaydelaney added minor Increment the minor version when merged release Create a release when this pr is merged labels Dec 5, 2024
@kaydelaney kaydelaney requested review from torkelo, bfmatei and a team December 5, 2024 17:19
@kaydelaney kaydelaney self-assigned this Dec 5, 2024
@kaydelaney kaydelaney requested review from axelavargas and removed request for a team December 5, 2024 17:19
parentState?: SceneCSSGridItemPlacement;
}

export class SceneCSSGridItem extends SceneObjectBase<SceneCSSGridItemState> {
Copy link
Member

Choose a reason for hiding this comment

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

I understand doing pocs here (if you need to make changes to VizPanelRenderer esp),

just be aware we have a special class in core for this CSS grid items, https://github.com/grafana/grafana/blob/main/public/app/features/dashboard-scene/scene/layout-responsive-grid/ResponsiveGridItem.tsx#L13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this wasn't a POC, I just factored it out from SceneCSSGridLayout

Copy link
Member

Choose a reason for hiding this comment

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

I understand, just know we are not using it dashboards.

I think a challenge with this work is what logic needs live in scenes vs the core repo implementation (and the ResponsiveGridLayoutManager)

@torkelo
Copy link
Member

torkelo commented Dec 6, 2024

Tried to test it with the css grid demo but just get an empty page
Screenshot 2024-12-06 at 06 32 49

@kaydelaney
Copy link
Contributor Author

@torkelo The css grid demo should work now - made some fixes.

@torkelo
Copy link
Member

torkelo commented Dec 10, 2024

@kaydelaney works really well!

I am not sure it helps or not but might be useful to go directly from this and explore how moving a panel between layouts would work (as as an early exploratory poc/test, to understand the problem etc) .

@torkelo
Copy link
Member

torkelo commented Dec 10, 2024

and by "between layouts" i mean between two separate CSS grids in this case (moving between CSS grid and the default grid is a separate problem, one we might have to solve as well)

const styles = useStyles2(getStyles, model.state);
const containerRef = useRef<HTMLDivElement>(null);
const oldDropZone = useRef<DropZone>();
const { activeItem, activeLayout, dropZone } = model.dragManager.useState();
Copy link
Collaborator

@mdvictor mdvictor Jan 16, 2025

Choose a reason for hiding this comment

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

Suggested change
const { activeItem, activeLayout, dropZone } = model.dragManager.useState();
const { activeItem, activeLayout, dropZone } = model.dragManager?.useState() ?? {};

I think we need something like this for cases where the drag manager isn't set. For example if I go to the demos listing page here it crashes because it expects the drag manager to exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Sorry, I was so focused on the demo app that I'd forgotten to check it didn't break other things. Will fix this asap

dropZone?: DropZone;
}

export class DragManager extends SceneObjectBase<DragManagerState> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move as much as we can of the drag and drop stuff to core, (potentially the have a separate CSS grid layout here), might move it back here if there is need for this in other scene apps but think we can iterate on it faster if it's in core, and we already have a specific CSSGridItem there

@kaydelaney
Copy link
Contributor Author

Some additional context to hopefully help with reviewing:

My main goal with the design was to have as much of the DOM and event-level stuff in one place as possible, so we don’t have to reimplement the logic for a whole bunch of different layouts.
So to that end, there are two main components of the design: We have a SceneLayout, in this case SceneCSSGridLayout, and we have DragManager, a scene behavior that handles all the nitty-gritty dom manipulation and events.
The idea is that a layout registers itself with the DragManager and attaches the DragManager's onDragStart method to whatever bit of html they decide is the grid item's "handle".
When the onDragStart method is invoked, the getDropZones method of any registered layout is invoked, returning an array of rectangles representing valid places the dragged grid item could be placed.
Some math is then done to figure out which of these rectangles is closest to the current cursor position, which is then selected as the “placeholder” position.
Both the grid item placeholder and the grid item being dragged are put into a react portal while dragging is active.

Right now the code for DragManager is still specific to SceneCSSGridLayout, but after some details are ironed out and the requirements for other layouts reveals itself, I think we can make it more generically useful by changing the SceneLayout interface as needed and using that instead.

@mdvictor Let me know if any additional details would be helpful!

@kaydelaney
Copy link
Contributor Author

In preparation for moving this code over to the main grafana repo, I've created a much smaller version of this PR that just adds the plumbing needed for when things are moved. #1028

Copy link
Contributor

@Sergej-Vlasov Sergej-Vlasov left a comment

Choose a reason for hiding this comment

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

Looks good, but getting the following errors and warnings in demo app when dragging:

SceneObject already has a parent set that is different from the new parent.

Uncaught TypeError: Cannot read properties of undefined (reading 'right')
at DragManager.onDrag (DragManager.js:110:62) ...

Comment on lines +67 to +71
const dragManager = findDragManager(this);
if (dragManager) {
this.dragManager = dragManager;
this.dragManager.registerLayout(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

getting typescript errors here. Might need a type cast in findDragManager() return
Type 'SceneObject<SceneObjectState> | SceneStatelessBehavior<any>' is not assignable to type 'DragManager | undefined'.

@kaydelaney
Copy link
Contributor Author

Grafana repo PR is up grafana/grafana#99386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged
Projects
Status: 🗂️ Needs Triage / Escalation
Development

Successfully merging this pull request may close these issues.

4 participants