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

Fix drag and drop panels in rows #663

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

mdvictor
Copy link
Contributor

@mdvictor mdvictor commented Apr 1, 2024

This fixes panel positioning glitching when drag/dropping a panel into a row.

There were 2 issues:

  • Moving panels into/out of rows caused a existing parent warning
  • After moving a panel into a row, when calling toggleRow the firstPanelYPos here would be wrongly selected because sortChildrenByPosition would not sort row children also, thus causing a glitch.

Fixes grafana/grafana#83292

📦 Published PR as canary version: 4.1.1--canary.663.8521906375.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.1.1--canary.663.8521906375.0
# or 
yarn add @grafana/scenes@4.1.1--canary.663.8521906375.0

@mdvictor mdvictor added patch Increment the patch version when merged release Create a release when this pr is merged labels Apr 1, 2024
Copy link
Contributor

@ivanortegaalba ivanortegaalba left a comment

Choose a reason for hiding this comment

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

The issue is fixed ✅

// after sorting by position, the row should have the children in the correct order [a,b]
const row = layout.state.children[0] as SceneGridRow;

expect(row.state.children[0].state.key).toEqual('a');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test here the grid position too? It would help to understand the expected result :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it wouldn't hurt. This test is mainly to showcase and verify rearrangement in the children array, but will add those too to be more explicit.

Comment on lines -220 to +222
const targetRow = target.clone({ children: [...target.state.children, newChild] });
const targetRow = target.clone();
targetRow.setState({ children: [...targetRow.state.children, newChild] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: Why this is different? Do we do something when updating the state that we don't do on creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we spread the old uncloned children, that have a parent( the target), with ...target.state.children instead of setting it's state with the cloned ones. Using those children threw a child already has a parent warning

@mdvictor mdvictor merged commit bdb6ffb into main Apr 2, 2024
3 checks passed
@mdvictor mdvictor deleted the mdvictor/fix-row-children-positioning branch April 2, 2024 11:24
@grafanabot
Copy link
Contributor

🚀 PR was released in v4.1.1 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

Drag and dropping panels from/into rows glitches panel positions in dashboard
3 participants