grafana / grafana

The open and composable observability and data visualization platform. Visualize metrics, logs, and traces from multiple sources like Prometheus, Loki, Elasticsearch, InfluxDB, Postgres and many more.
https://grafana.com
GNU Affero General Public License v3.0
60.72k stars 11.61k forks source link

Canvas: Add support to rotate a group of elements #87358

Closed nmarrs closed 2 weeks ago

nmarrs commented 2 weeks ago

Add support to rotate elements in a group - current UX indicates this is possible so seems like a bug / not a great experience for users

Before

https://github.com/grafana/grafana/assets/22381771/81571ac7-e15f-4672-802a-d10325adfb4e

After

https://github.com/grafana/grafana/assets/22381771/b42feabf-58c1-462f-a4ec-d192ec15d3fc

After (with connections!) - same behavior as rotating a single element / no regressions

https://github.com/grafana/grafana/assets/22381771/71942ccd-e3a7-46ef-ad94-7e28dcf74eca

Fixes https://github.com/grafana/grafana/issues/84961

grafana-delivery-bot[bot] commented 2 weeks ago

Hello @nmarrs! Backport pull requests need to be either:

Please, if the current pull request addresses a bug fix, label it with the type/bug label. If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label. If the pull request modifies CI behaviour, please add the type/ci label. If none of the above applies, please consider removing the backport label and target the next major/minor release. Thanks!

grafana-delivery-bot[bot] commented 2 weeks ago

This PR must be merged before a backport PR will be created.

drew08t commented 2 weeks ago

The rotation works as expected for me, but I'm seeing some strange things happening to styling on multiple panels. Notice the background colors: image Not sure if it's a regression from this PR (doubt it) or something else is happening.

Also, a couple of notes:

  1. In general this is a good step forward, but I think we need to revisit group rotation in the near future. For example, should we have all elements rotate about a single point? Obviously the handling of anchors needs to be improved as well in a follow-up.
  2. Why are there two rotation control handles?
nmarrs commented 2 weeks ago

The rotation works as expected for me, but I'm seeing some strange things happening to styling on multiple panels. Notice the background colors: image Not sure if it's a regression from this PR (doubt it) or something else is happening.

Hm I noticed regression as well, I think it is pretty recent regression that we probably will want to get to the bottom of sooner than later

  1. In general this is a good step forward, but I think we need to revisit group rotation in the near future. For example, should we have all elements rotate about a single point? Obviously the handling of anchors needs to be improved as well in a follow-up.

How else might a group of elements rotate? 🤔 I think moveable has support for changing the relative "center" of rotation but not sure what use cases / value this would add in this case

Re: connection anchor rendering issue we have this issue to follow up on this

  1. Why are there two rotation control handles?

See this issue and the PR fix that addressed it :)

drew08t commented 2 weeks ago

The rotation works as expected for me, but I'm seeing some strange things happening to styling on multiple panels. Notice the background colors: image Not sure if it's a regression from this PR (doubt it) or something else is happening.

Hm I noticed regression as well, I think it is pretty recent regression that we probably will want to get to the bottom of sooner than later

  1. In general this is a good step forward, but I think we need to revisit group rotation in the near future. For example, should we have all elements rotate about a single point? Obviously the handling of anchors needs to be improved as well in a follow-up.

How else might a group of elements rotate? 🤔 I think moveable has support for changing the relative "center" of rotation but not sure what use cases / value this would add in this case

Re: connection anchor rendering issue we have this issue to follow up on this

  1. Why are there two rotation control handles?

See this issue and the PR fix that addressed it :)

For me, it's more intuitive to rotate a group of elements about a common point, perhaps the centroid of the selected bounding boxes could work.

nmarrs commented 2 weeks ago

For me, it's more intuitive to rotate a group of elements about a common point, perhaps the centroid of the selected bounding boxes could work.

Hm like move the position of the elements around a common point vs modify each element's rotation in place based on the delta of rotation being applied to the overall group selection box? I think I understand where you are coming from - I think this initial implementation is probably "good enough" / doesn't appear totally broken to user etc but yeah we can explore improving this for sure 👍

Implementing this would be a bit more involved with actually modifying the positioning of each element vs just the element's rotation property

drew08t commented 2 weeks ago

The rotation works as expected for me, but I'm seeing some strange things happening to styling on multiple panels. Notice the background colors: image Not sure if it's a regression from this PR (doubt it) or something else is happening.

Hm I noticed regression as well, I think it is pretty recent regression that we probably will want to get to the bottom of sooner than later

  1. In general this is a good step forward, but I think we need to revisit group rotation in the near future. For example, should we have all elements rotate about a single point? Obviously the handling of anchors needs to be improved as well in a follow-up.

How else might a group of elements rotate? 🤔 I think moveable has support for changing the relative "center" of rotation but not sure what use cases / value this would add in this case

Re: connection anchor rendering issue we have this issue to follow up on this

  1. Why are there two rotation control handles?

See this issue and the PR fix that addressed it :)

For me, it's more intuitive to rotate a group of elements about a common point, perhaps the centroid of the selected bounding boxes could work.

Hm like move the position of the elements around a common point vs modify each element's rotation in place based on the delta of rotation being applied to the overall group selection box? I think I understand where you are coming from - I think this initial implementation is probably "good enough" / doesn't appear totally broken to user etc but yeah we can explore improving this for sure 👍

Implementing this would be a bit more involved with actually modifying the positioning of each element vs just the element's rotation property

Yep, we would need to apply some transformations, shouldn't be too horrible, but definitely worth exploring and following up on later.