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

[Dashboard][Collapsable Panels] Move panel element directly #190647

Closed
Tracked by #190342
ThomThomson opened this issue Aug 15, 2024 · 2 comments · Fixed by #196756
Closed
Tracked by #190342

[Dashboard][Collapsable Panels] Move panel element directly #190647

ThomThomson opened this issue Aug 15, 2024 · 2 comments · Fixed by #196756
Assignees
Labels
Feature:Dashboard Dashboard related features 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:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@ThomThomson
Copy link
Contributor

ThomThomson commented Aug 15, 2024

In kbn grid layout the drag preview follows the mouse position and appears above the grid. The actual element being dragged snaps to its final destination in the grid under the drag preview.

In react grid layout and many other products, the actual element follows the mouse position directly during a drag, and the drag preview appears behind the actual element to show the final destination of the drag operation.

We should consider swapping this behaviour.

react grid layout kbn grid layout
react grid kbn grid

POC here

@botelastic botelastic bot added the needs-team Issues missing a team label label Aug 15, 2024
@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. and removed needs-team Issues missing a team label labels Aug 15, 2024
@teresaalvarezsoler
Copy link

I can see how the new behaviour can be also valid. We should test if it is problematic for users before making any changes. Let's keep it open for now but leave it till the end of the project.

@Heenawter Heenawter self-assigned this Oct 15, 2024
@Heenawter
Copy link
Contributor

Heenawter commented Oct 15, 2024

In order to properly add an "on drop" event listener, I believe this is actually necessary and not a nice-to-have - I am assigning it to myself to explore further since, if this is the case, it is blocking my API work.

As a summary for why I need this, as part of the API work, I was thinking through what events the Dashboard (or any consumer of our grid layout) would need access to - and I realized we would need an onLayoutChange callback. However, our current "on layout change" event fires any time an element is moved/updated (i.e. any time a layout change happens from a collision, and not only when you drop the item you are dragging). For the Dashboard use case, this is not ideal, because that means that the unsaved changes would be triggered before the panel is dropped, which is different than the current behaviour.

The problem is, by dragging a preview and not the actual element, the onMouseUp event is not being triggered properly for the drag handler icon - so we have no way of knowing when a panel has been dropped. We are currently getting around this by adding an onMouseUp listener to the entire document, but this fires for every mouse up event - i.e. every click - not just for panel drag events. And we don't want our onLayoutChange to be called on every click.

Hence, why I am currently exploring dragging the real element as a solution.

There is a potential workaround where we still attach the onMouseUp listener to the document, but in this listener, we only call the onLayoutChange when the layout is different - however, this isn't ideal IMO. So we'll see how far I can get with this, instead.

@Heenawter Heenawter added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 17, 2024
@Heenawter Heenawter added loe:small Small Level of Effort and removed loe:medium Medium Level of Effort labels Oct 18, 2024
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 22, 2024
…astic#196756)

Closes elastic#190647

## Summary

This PR swaps the interaction of `kbn-grid-layout` so that you are now
dragging/resizing the **actual element** rather than the preview - i.e.
the preview now shows the **real** / grid aligned location (i.e. where
the panel will land once the interaction stops), while the element shows
the non-grid-aligned location.

**Dragging**

| Before | After |
|--------|--------|
| ![Oct-18-2024
09-10-52](https://github.com/user-attachments/assets/f117124d-3200-4c7b-a5f7-6a4bc767ebff)
| ![Oct-18-2024
09-07-25](https://github.com/user-attachments/assets/483d481a-a752-4455-b9bd-2d89ec273454)
|

**Resizing**

| Before | After |
|--------|--------|
| ![Oct-18-2024
09-11-21](https://github.com/user-attachments/assets/64e4314d-b641-4b0c-a4a9-93e3f0d21cbc)
| ![Oct-18-2024
09-07-55](https://github.com/user-attachments/assets/755be726-38bc-475b-a85d-7696262c4b4f)
|

This serves as more than just a visual update - because we are dragging
the real element, the mouse stays "locked" to the drag and/or resize
handler, which means we have introduced the possibility for an `onDrop`
event. This is necessary in order to keep the current "unsaved changes"
behaviour on Dashboard, where changes are triggered only once the panel
is actually **dropped** and not when other panels move as a consequence
of a drag event.

To make this possible, I also removed the `GridOverlay` component - it
was creating a transparent `div` **over the entire grid** on
interaction, which meant that focus was lost as soon as the interaction
started. If we want to restore the "scroll up" and "scroll down" buttons
(which we were unsure about, anyway), we would need to rethink this
(i.e. just render two fixed-position buttons without any overlay).

### Checklist

- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit cb56679)
@Heenawter Heenawter removed the Feature:Dashboard Dashboard related features label Nov 21, 2024
@Heenawter Heenawter added the Feature:Dashboard Dashboard related features label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features 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:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants