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 field name placement option #75932

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

nmarrs
Copy link
Contributor

@nmarrs nmarrs commented Oct 3, 2023

Updated behavior (with namePlacement)

up.to.date.v2.mov

Auto (before)

auto.original.behavior.mov

Auto / Top / Left

all.of.the.options.mov

Fixes #75928

@nmarrs nmarrs added this to the 10.2.x milestone Oct 3, 2023
@nmarrs nmarrs self-assigned this Oct 3, 2023
@nmarrs nmarrs requested review from a team, imatwawana and Eve832 as code owners October 3, 2023 22:17
@nmarrs nmarrs requested review from joshhunt, eledobleefe and mckn and removed request for a team October 3, 2023 22:17
@grafana-pr-automation grafana-pr-automation bot added type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project area/panel/gauge area/frontend labels Oct 3, 2023
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

Code looks great! Added a question about the naming.

@torkelo
Copy link
Member

torkelo commented Oct 4, 2023

@nmarrs should not be named "name placement" option? it does not look like it's changing the value placement

Copy link
Collaborator

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

Thanks for updating! Made some suggestions.

@nmarrs nmarrs requested review from mckn and imatwawana October 4, 2023 23:49
@adela-almasan
Copy link
Contributor

Tested locally and works well! I noticed 2 things - the option is not shown for hidden Value display (I don't think they're related) but it's displayed for vertical orientation although it doesn't do anything. Maybe hide it on vertical orientation and disconnect it from value display?

Screen.Recording.2023-10-04.at.10.30.12.PM.mov

@nmarrs
Copy link
Contributor Author

nmarrs commented Oct 5, 2023

@nmarrs should not be named "name placement" option? it does not look like it's changing the value placement

😅 not sure what I was thinking - updated the naming to be Field label placement instead

@leeoniya
Copy link
Contributor

leeoniya commented Oct 5, 2023

😅 not sure what I was thinking - updated the naming to be Field label placement instead

"Field label" might be confusin also, since fields actually have labels, and this is not that 😅 . @torkelo suggested "Name placement", which is probably better? what do we do in other panels?

in bar chart we call it both X-axis labels and X-axis tick labels (probably should remove "tick"). it's tricky there because BarChart can show timestamps on X, so calling it Names felt weird.

image

In StateTimeline we don't call them anything because we dont have any options for them yet, but Name/Names is probably what we'll go with when we add options for truncation and alignment control. We already use "Axis label" in other panels, which is something different. Naming is hard :(

@nmarrs
Copy link
Contributor Author

nmarrs commented Oct 5, 2023

😅 not sure what I was thinking - updated the naming to be Field label placement instead

"Field label" might be confusin also, since fields actually have labels, and this is not that 😅 . @torkelo suggested "Name placement", which is probably better? what do we do in other panels?

yes - naming is hard 🙃 I think I would be fine with any of the following:
fieldNamePlacement
gaugeLabelPlacement (this one feels okish / more descriptive as to what it actually is)
and maybe just namePlacement (doesn't seem as descriptive 🤷‍♂️)

@nmarrs nmarrs changed the title Bar Gauge: Add value placement option Bar Gauge: Add field name placement option Oct 5, 2023
@nmarrs
Copy link
Contributor Author

nmarrs commented Oct 5, 2023

Maybe hide it on vertical orientation and disconnect it from value display?

done - thanks for catching this / the suggestion @adela-almasan :)

- **Text color -** Value color is default text color.
- **Hidden -** Values are hidden.

### Field label placement
Copy link
Member

Choose a reason for hiding this comment

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

I think name is better term than field label. In the sister panel Stat we refer to it as Name , and in settings we refer to it as "Display name" , never label.

So think "Name placement" as the option name is best

@nmarrs nmarrs requested a review from torkelo October 5, 2023 06:30
Copy link
Collaborator

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@adela-almasan adela-almasan left a comment

Choose a reason for hiding this comment

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

@nmarrs nmarrs merged commit b94d06b into main Oct 5, 2023
21 checks passed
@nmarrs nmarrs deleted the bar-gauge-add-label-placement-option branch October 5, 2023 16:31
@adela-almasan adela-almasan mentioned this pull request Oct 10, 2023
20 tasks
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/panel/bargauge area/panel/gauge no-backport Skip backport of PR type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bar Gauge: Add option for label placement
7 participants