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

Table Panel: Add ability to use text color for value or hide value in gauge cell #61477

Merged
merged 28 commits into from Mar 10, 2023
Merged

Table Panel: Add ability to use text color for value or hide value in gauge cell #61477

merged 28 commits into from Mar 10, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Jan 13, 2023 •
edited by codeincarnate

So now that we have cell options we can add more options to specific cell modes.

This option is something I have been wanting to add for ages (and @ryantxu as well)

  •  Add new value display mode: Color text (old default) | Text color | Hidden
  • Fix typings for TableCellOptions, the current typing could not take advantage of typescript union typing/discriminator as the TableAutoCellOptions supported all TableCellDisplayMode for it's type property. This break the union type benefits (of being able to check type and get the other props part of that type automatically). But was not able to type this in cue efficiently, had to create a specific type for each enum value. Due to cuetsy not supporting multiple enum values for a property.
  • Fixed sentence casing for some property names
  • Fixed double button margin in the editor
  • Update react table gdev test dashboard
  • Updated the logic in BarGaugeCell for supporting the old displayMode, could need extra review, I did not understand the existing code

Open question

  • Should we default to text color? ok breaking change or migrate current options to "color" and then make default text color

Todo:

  • Update bar gauge panel with options
  • Try to add some tests for BarGauge
  • Docs

Another value mode option we can explore in the future would be "inside" where the value is shown inside the bar

Screenshot 2023-01-13 at 11 25 41

Screenshot 2023-01-13 at 12 20 51

Fixes #45442, #40925

@torkelo torkelo requested a review from a team January 13, 2023 11:34
@torkelo torkelo requested review from a team as code owners January 13, 2023 11:34
@torkelo torkelo requested review from zoltanbedi, mdvictor, axelavargas, polibb, JoaoSilvaGrafana and academo and removed request for a team January 13, 2023 11:34
@torkelo torkelo added this to the 9.4.0 milestone Jan 13, 2023
@torkelo torkelo added add to changelog no-backport Skip backport of PR labels Jan 13, 2023
@torkelo torkelo changed the title Table: Tweaks to cell options types, new gauge cell value mode option Table: New gauge cell value mode option to use text color for value or hide value Jan 13, 2023
mdvictor
mdvictor requested changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@mdvictor mdvictor left a comment •
edited

Choose a reason for hiding this comment

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

Works great! I left some comments regarding code changes / nits.

@codeincarnate codeincarnate requested a review from a team as a code owner March 8, 2023 07:53
@codeincarnate
Copy link
Collaborator

@mdvictor think I got all of the above resolved. Would appreciate another look! 🙏

@codeincarnate codeincarnate added this to the 9.5.0 milestone Mar 8, 2023
@mdvictor mdvictor self-requested a review March 9, 2023 12:03
mdvictor
mdvictor approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

LGTM

@codeincarnate codeincarnate changed the title Table: New gauge cell value mode option to use text color for value or hide value Table Panel: Add ability to use text color for value or hide value in gauge cell Mar 9, 2023
sdboyer
sdboyer reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

OK, i've fixed the validation errors on devenv. Here's what i found.

For devenv/dev-dashboards/panel-table/table_tests_new.json, there were two issues, fixed in this commit. For devenv/dev-dashboards/panel-bargauge/panel_tests_bar_gauge.json, some fixes in this commit. I've made inline comments discussing each change. Pretty much all of them merit follow-ups.

devenv/dev-dashboards/panel-table/table_tests_new.json Outdated Show resolved Hide resolved
devenv/dev-dashboards/panel-table/table_tests_new.json Outdated Show resolved Hide resolved
devenv/dev-dashboards/panel-bargauge/panel_tests_bar_gauge.json Outdated Show resolved Hide resolved
kinds/dashboard/dashboard_kind.cue Show resolved Hide resolved
@torkelo torkelo merged commit 73ce20a into main Mar 10, 2023
10 checks passed
@torkelo torkelo deleted the table-bar-gauge-options branch March 10, 2023 13:41
eleijonmarck pushed a commit that referenced this pull request Mar 13, 2023
@torkelo @codeincarnate
@sdboyer @eleijonmarck
Table Panel: Add ability to use text color for value or hide value in…
73454ec 
… gauge cell (#61477)

* BarGauge: New value options

* Fix typings for cell options, add new value mode option for bar gauge cells

* Add BarGauge panel option, tests, and update test dashboard

* Updated

* Added default

* Goodbye trusty console.log

* Update

* Merge changes from main

* Update docs

* Add valuemode doc changes

* Update gdev dashboard

* Update valueMode symbol name to valueDisplayMode

* Use Enums as Opposed to literals, don't calculate values when hidden

* Remove double import

* Fix tests

* One more test fix

* Remove erroneous targets field, fix type of maxDataPoints

* Strip nulls and add index field to Thresholds

* Gen cue

* remove bad targets again

* Fixes

---------

Co-authored-by: Kyle Cunningham <kyle@codeincarnate.com>
Co-authored-by: sam boyer <sdboyer@grafana.com>
@codeincarnate
Copy link
Collaborator

Thanks @sdboyer and @torkelo. Very nice to have this merged in and have the corresponding (long requested) issues closed out 🎉

@torkelo
Copy link
Member Author

torkelo commented Mar 15, 2023

@codeincarnate great!

@zoltanbedi zoltanbedi mentioned this pull request Mar 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdboyer sdboyer sdboyer left review comments

@mdvictor mdvictor mdvictor approved these changes

@codeincarnate codeincarnate Awaiting requested review from codeincarnate

@ryantxu ryantxu Awaiting requested review from ryantxu

Assignees
No one assigned
Labels
add to changelog area/backend area/frontend area/panel/table no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
9.5.0
Development

Successfully merging this pull request may close these issues.

Table: Bar gauge cell display mode with varying value lengths in a narrow column creates miss-alignment
5 participants
@torkelo @codeincarnate @grafanabot @sdboyer @mdvictor