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

Bar Gauge: Add max height option #76042

Merged
merged 18 commits into from
Nov 7, 2023
Merged

Bar Gauge: Add max height option #76042

merged 18 commits into from
Nov 7, 2023

Conversation

Develer
Copy link
Contributor

@Develer Develer commented Oct 5, 2023

What is this feature?

This PR add new option for max height of bar in horizontal orientation.

Fixes #75982

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

For doc changes see #76240

@Develer Develer added add to changelog no-backport Skip backport of PR area/dataviz Anything that relates to Data Visualisation work but is not specific to one panel labels Oct 5, 2023
@Develer Develer added this to the 10.2.x milestone Oct 5, 2023
@Develer Develer requested a review from a team as a code owner October 5, 2023 13:02
@Develer Develer self-assigned this Oct 5, 2023
@Develer Develer requested review from a team and Eve832 as code owners October 5, 2023 13:02
@Develer Develer requested review from L-M-K-B, eledobleefe and jackw and removed request for a team October 5, 2023 13:02
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-ui has possible breaking changes (more info)

Console output
Read our guideline

@github-actions github-actions bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Oct 5, 2023
@Develer Develer removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Oct 5, 2023
@github-actions github-actions bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Oct 6, 2023
@nmarrs nmarrs removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Oct 6, 2023
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.

Overall looks good! We should also update the bar gauge docs (index.md) file with this added property - that can be done in a follow-up PR though ;)

@torkelo
Copy link
Member

torkelo commented Oct 9, 2023

Is this really needed? Been many years like without any request for this option.

@Develer
Copy link
Contributor Author

Develer commented Oct 31, 2023

@nmarrs
I was so obsessed with reducing the number of options that I decided to make layout orientation and bar sizing options dependent on each other. And now I see that it doesn't make much sense, thanks for pointing it out.

path: 'minVizWidth',
name: 'Min width',
description: 'Minimum column width',
description: 'Minimum column width (vertical orientation)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmarrs There is one point to discuss.

I want to show specific sizing options for selected layout orientation. For example, to display Min width for vertical. layout orientation only, to avoid options overwhelming.

I didn't find any proper way to show/hide sizing options for specific layout orientations. For example, if there is auto layout orientation we can't say the exact orientation, so we can't decide which sizing options to show or hide. So right now it's gonna be displayed all sizing options both for vertical and horizontal layout.
Btw, the same situation happening with the Name placement option.

I decided to add an extra description to each sizing option. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is reasonable :) I think this is good to show off to Torkel / get this thoughts / approval

max: 300,
step: 1,
},
showIf: (options) => options.sizing === BarGaugeSizing.Manual,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can still make the manual options a function of layout orientation right?

for auto we show all

for horizontal we show max / min height

for vertical we just show width

With this I think we can keep the description explicitely saying what they are for / apply, which is especially useful in auto mode

@Develer
Copy link
Contributor Author

Develer commented Nov 3, 2023

Hi @torkelo,
Here is an implementation we came up with. Showing/hiding sizing options depends on orientation and auto/manual sizing mode. We are also showing all sizing options in manual sizing mode and auto layout orientation. Let us know what you think.

Screen.Recording.2023-11-03.at.12.19.48.mov

@torkelo
Copy link
Member

torkelo commented Nov 3, 2023

@Develer looks great

@Develer Develer removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 6, 2023
@Develer Develer enabled auto-merge (squash) November 6, 2023 19:47
@github-actions github-actions bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 6, 2023
@Develer Develer merged commit 4b87f38 into main Nov 7, 2023
19 of 20 checks passed
@Develer Develer deleted the 75982-add-max-height branch November 7, 2023 05:11
@grafana-delivery-bot grafana-delivery-bot bot modified the milestones: 10.2.x, 10.3.x Nov 7, 2023
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
@Leviathan09
Copy link

So now that we have "Bar Size" i have to update all my Panels and Dashboards with a new Min height value to correct that poor implementation
Why would you keep the "Auto" function when it doesn't work?
I have to edit now every single Panel and all my Dashboards.

Not amused !

Comment on lines -27 to +31
minVizHeight: 10,
minVizWidth: 0,
maxVizHeight: 300,
minVizHeight: 75,
minVizWidth: 75,
Copy link
Member

Choose a reason for hiding this comment

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

@nmarrs @Develer these changes sadly these changes to the defaults that are still used in "auto" mode completely and utterly break the auto mode. The sad part is that we cannot just restore the values to fix it as any saved dashboard will be saved with these new values.

but maybe the fix is just to not use these defaults in auto mode.

Copy link
Member

Choose a reason for hiding this comment

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

wait, no we can just change these. even though they are persisted the defaults are directly used when sizing is set to auto

@nmarrs
Copy link
Contributor

nmarrs commented Jan 2, 2024

@Leviathan09 we are sorry for introducing this bug / regression and for the headache that it caused :/ This was not our intention and we have merged a PR to fix this asap (will be in v10.3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/dataviz Anything that relates to Data Visualisation work but is not specific to one panel area/frontend area/panel/gauge area/panel/stat levitate breaking change A label indicating a breaking change and assigned by Levitate. no-backport Skip backport of PR type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bar gauge: Add max height option
6 participants