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.

Sign up for GitHub

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

Jump to bottom

Migrations: Graph (old) percent stacked #84335

Merged
merged 5 commits into from Apr 25, 2024
Merged

Migrations: Graph (old) percent stacked #84335

merged 5 commits into from Apr 25, 2024

Conversation

leeoniya
Copy link
Contributor

Fixes #84263

test with dashboard in #84263 (comment)

this also switches to using hex colors to fix the hover point stroke getting incorrect color due to some migrated colors being rgb() and our alpha() setting function expects #hex color strings.

@leeoniya leeoniya added area/panel/graph type/angular-2-react Angular -> React migration area/panel/timeseries The main time series Graph panel no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 13, 2024
@leeoniya leeoniya self-assigned this Mar 13, 2024
@leeoniya leeoniya requested a review from a team as a code owner March 13, 2024 03:16
@leeoniya leeoniya requested review from codeincarnate, Develer and adela-almasan and removed request for a team March 13, 2024 03:16
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.0.x milestone Mar 13, 2024
@leeoniya leeoniya requested review from nmarrs and removed request for codeincarnate March 13, 2024 03:16
leeoniya
leeoniya commented Mar 13, 2024
View reviewed changes
public/app/plugins/panel/timeseries/migrations.ts
Comment on lines +364 to +368
// TimeSeries currently uses 0-1 for percent, so allowing zero leaves only top and bottom ticks.
// removing it feels better. probably should fix in TimeSeries, but let's kick it down the road
if (y1.decimals === 0) {
delete y1.decimals;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

likely controversial, but this is much simpler than fixing TimeSeries decimals setting in percent stacked mode to understand that 0 decimals is specifically in the 0-100 and not 0-1 range. i think generally this will do the correct thing. let's see what feedback we get before dedicating more time to this than needed.

nmarrs
nmarrs approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

LGTM - maybe it worth adding a few tests covering the color / other fix in this PR? :)

@ashharrison90 ashharrison90 modified the milestones: 11.0.x, 11.1.x Mar 26, 2024
@leeoniya
Copy link
Contributor Author

LGTM - maybe it worth adding a few tests covering the color / other fix in this PR? :)

reverted for now. will do in separate PR or handle this properly in the TimeSeries panel instead of migration code.

@leeoniya leeoniya added backport v11.0.x Mark PR for automatic backport to v11.0.x type/bug and removed no-backport Skip backport of PR labels Apr 25, 2024
@grafana-delivery-bot grafana-delivery-bot
Copy link
Contributor

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

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

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 grafana-delivery-bot
Copy link
Contributor

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

baldm0mma
baldm0mma approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

Builds and behaves locally as expected. :shipit:

@leeoniya leeoniya merged commit d1b6784 into main Apr 25, 2024
14 checks passed
@leeoniya leeoniya deleted the leeoniya/graph-old-migrate-percent-stacked branch April 25, 2024 17:51
grafana-delivery-bot bot pushed a commit that referenced this pull request Apr 25, 2024
@leeoniya @grafana-delivery-bot
Migrations: Graph (old) percent stacked (#84335)
125e37e 
(cherry picked from commit d1b6784)
@grafana-delivery-bot grafana-delivery-bot bot mentioned this pull request Apr 25, 2024
Merged
leeoniya added a commit that referenced this pull request Apr 25, 2024
@grafana-delivery-bot @leeoniya
[v11.0.x] Migrations: Graph (old) percent stacked (#86945)
e151121 
Migrations: Graph (old) percent stacked (#84335)

(cherry picked from commit d1b6784)

Co-authored-by: Leon Sorokin <leeoniya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmarrs nmarrs nmarrs approved these changes

@baldm0mma baldm0mma baldm0mma approved these changes

@Develer Develer Awaiting requested review from Develer Develer is a code owner automatically assigned from grafana/dataviz-squad

@adela-almasan adela-almasan Awaiting requested review from adela-almasan

Assignees

@leeoniya leeoniya

Labels
area/frontend area/panel/graph area/panel/timeseries The main time series Graph panel backport v11.0.x Mark PR for automatic backport to v11.0.x no-changelog Skip including change in changelog/release notes type/angular-2-react Angular -> React migration type/bug
Projects
None yet
Milestone
11.1.x
Development

Successfully merging this pull request may close these issues.

Improve auto migration from Graph panel to Time Series panel
4 participants
@leeoniya @nmarrs @baldm0mma @ashharrison90