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] New layout engine #174132

Merged
merged 33 commits into from
Aug 15, 2024
Merged

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Jan 2, 2024

Summary

This PR contains a new performant and simple drag & drop layout engine for Kibana which uses HTML5 and CSS and and has no external dependencies.

Closes #190372

Kbn grid layout

The new layout engine is isolated in its own package @kbn/grid-layout, and the PR also contains an example plugin showing its use.

Sections

How to test

  1. Run kibana with yarn start --run-examples
  2. Navigate to Analytics -> Developer examples
  3. Open the Grid example.
  4. ❓ ❓ ❓ ❓
  5. Profit

Known differences

There are a few differences when comparing kbn grid layout and our current layout engine react grid layout.

No animations on subsequent panel moves

When a panel collides with another and pushes it out of the way, kbn grid layout animates this. Because of our direct usage of CSS grid, we are unable to animate these events. I.e. grid-column and grid-row properties do not support transition.

react grid layout kbn grid layout
Aug-12-2024 17-40-13 Aug-12-2024 17-37-21

This difference can make kbn grid layout appear snappier, but much less smooth. This would be a very difficult difference to overcome, as we would need to move away from css grid and calculate pixel locations and sizes on the fly.

Difficulty moving a larger panel under a smaller one

react grid layout has special code for handling a case where a panel is moved down onto another. This mostly manifests in an earlier swap when moving a large panel underneath a smaller one. kbn grid layout does not have this code, which. means users must drag a panel actually below the panel underneath for the swap to happen.

react grid layout kbn grid layout
Aug-12-2024 19-19-54 Aug-12-2024 19-19-13

This is somewhat mitigated by the fact that the drag preview doesn't block visibility of the panel underneath. Fixing this difference would require adding a small edge case to our sorting algorithm, and wouldn't be too difficult of a change.

Events are canceled when moving outside the browser window

react grid layout allows the user to drag outside the browser window. The panel will continue to be dragged as if it was still following the mouse. kbn grid layout does not do allow this, and dragging outside of the browser window will cancel the drag operation.

react grid layout kbn grid layout
Aug-12-2024 19-27-44 Aug-12-2024 19-28-16

This may cause frustration as the user needs to be careful about where they drag the panels. This could be fixed by digging into the internals of React Grid Layout's event handling to find differences, and would not be too much trouble as React Grid Layout also uses HTML5 drag and drop.

@ThomThomson ThomThomson changed the title [Spacetime] HTML Drag Drop Grid POC [Dashboard] New layout engine POC Jun 7, 2024
@ThomThomson ThomThomson changed the title [Dashboard] New layout engine POC [Dashboard] New layout engine Aug 12, 2024
@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. release_note:skip Skip the PR/issue when compiling release notes ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project and removed ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project labels Aug 13, 2024
@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson ThomThomson marked this pull request as ready for review August 14, 2024 14:07
@elasticmachine
Copy link
Contributor

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

@nreese nreese self-requested a review August 15, 2024 13:28
@nreese
Copy link
Contributor

nreese commented Aug 15, 2024

The grid layout feels really snappy and smooth.

One negative thing I have noticed when comparing the new layout system to the old layout system is that the shadow element (green box) and real element are switched.

In the old system, the real element is attached to your mouse movements and is on top of the shadow element (red box). This feels really natural and easy to track since the real element is opaque and the shadow element is mostly covered up.

In the new system, the shadow element is attached to your mouse movements and is on top of the real element. I found this unnatural. The shadow element is translucent and its easy for your eye to get stuck on the real element underneath which is not attached to mouse movements. Moving the panel around feels less natural because your eye is focused on the real element which does not directly track mouse movement, making shadow element more distracting.

Is there a reason for this change?

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.

Are there plans for dark mode support?
Screenshot 2024-08-15 at 7 56 50 AM

@@ -0,0 +1,3 @@
# Grid Example

This temporary example plugin will be used to build out the new layout engine for Dashboards.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets not make this temporary. It would be nice to have a playground for the layout engine outside of dashboard. Plus, its a good learning resource without all the noise of dashboard implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 75a4e51

examples/grid_example/public/app.tsx Show resolved Hide resolved
developerExamples.register({
appId: GRID_EXAMPLE_APP_ID,
title: gridExampleTitle,
description: `This temporary example plugin will be used to build out the new layout engine for Dashboards.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

again, lets change wording to call it a playground for the layout engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 75a4e51

{gridLayout.map((rowData, rowIndex) => {
return (
<GridRow
key={rowIndex}
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 using rowData.title as key since index is not a recommended key?

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 catch! Addressed in 75a4e51

@ThomThomson
Copy link
Contributor Author

In the new system, the shadow element is attached to your mouse movements
Is there a reason for this change?

Good callout. This change was made mostly for its implementation simplicity. It's much more natural from a code perspective to move the actual element around in CSS grid coordinates and have the ghost follow the mouse position. I also felt like it gave a better preview of what the grid will look like on drop.

That said, I can see how this could feel jarring. To fix this problem we could either:

  1. Mitigate this with design elements - i.e. maybe the panel being dragged gets less contrast and the shadow element gets more opaque?
  2. Look into actually swapping the elements. This will likely make the implementation more complex, but it's definitely doable!

@andreadelrio what do you think about this drawback?

Are there plans for dark mode support?

Looks like this is a problem with the example plugin. I will look into propagating dark mode into it. Dark mode works great with the engine in Dashboard.

Aug-15-2024 10-31-01

@nreese
Copy link
Contributor

nreese commented Aug 15, 2024

Mitigate this with design elements - i.e. maybe the panel being dragged gets less contrast and the shadow element gets more opaque?

This is a good place to start

@andreadelrio
Copy link
Contributor

its easy for your eye to get stuck on the real element underneath which is not attached to mouse movements. Moving the panel around feels less natural because your eye is focused on the real element which does not directly track mouse movement, making shadow element more distracting.

Is there a reason for this change?

In the new system, the shadow element is attached to your mouse movements
Is there a reason for this change?

Good callout. This change was made mostly for its implementation simplicity. It's much more natural from a code perspective to move the actual element around in CSS grid coordinates and have the ghost follow the mouse position. I also felt like it gave a better preview of what the grid will look like on drop.

That said, I can see how this could feel jarring. To fix this problem we could either:

  1. Mitigate this with design elements - i.e. maybe the panel being dragged gets less contrast and the shadow element gets more opaque?
  2. Look into actually swapping the elements. This will likely make the implementation more complex, but it's definitely doable!

@andreadelrio what do you think about this drawback?

I would consider this a significant drawback of the new layout engine. I think the behavior should be as before: mouse movement attached to real element, shadow shown behind real element. This is also the behavior I've seen in all the references I've reviewed. I don't think design tweaks are going to get us very far here and would encourage the team to consider #2. cc @teresaalvarezsoler

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.

This is an excellent start to the layout engine. Really clean solution. We can work out details in follow on PRs since this PR does not merge any user facing features.
code review and tested in chrome

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@ThomThomson
Copy link
Contributor Author

I've created two follow up issues:

one for the strange scrolling behaviour and one for the swapping of the ghost and the actual element when dragging.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #14 / DataTableColumnHeader should render a correct token

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/grid-layout - 16 +16

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/grid-layout - 1 +1
Unknown metric groups

API count

id before after diff
@kbn/grid-layout - 16 +16

ESLint disabled line counts

id before after diff
@kbn/grid-layout - 2 +2

Total ESLint disabled count

id before after diff
@kbn/grid-layout - 2 +2

History

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

@ThomThomson ThomThomson merged commit 7290824 into elastic:main Aug 15, 2024
41 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Aug 15, 2024
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:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:large Large Level of Effort Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. 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.

[Dashboard][Collapsable Panels] Create new Layout Engine Package
7 participants