-
Notifications
You must be signed in to change notification settings - Fork 669
Bug 1804143: Monitoring Dashboards: Remove broken bar chart links & clean up #4454
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
Changes from all commits
96758f0
8c6eccf
2b39800
97ffade
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,13 +23,14 @@ const PADDING_RATIO = 1 / 3; | |
| export const BarChart: React.FC<BarChartProps> = ({ | ||
| barSpacing = 15, | ||
| barWidth = DEFAULT_BAR_WIDTH, | ||
| title, | ||
| query, | ||
| data = [], | ||
| isLink = true, | ||
| LabelComponent, | ||
| loading = false, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not new, but do we need the default here since |
||
| query, | ||
| theme = getCustomTheme(ChartThemeColor.blue, ChartThemeVariant.light, barTheme), | ||
| title, | ||
| titleClassName, | ||
| loading = false, | ||
| LabelComponent, | ||
| }) => { | ||
| const [containerRef, width] = useRefWidth(); | ||
|
|
||
|
|
@@ -46,7 +47,7 @@ export const BarChart: React.FC<BarChartProps> = ({ | |
| return ( | ||
| <PrometheusGraph ref={containerRef} title={title} className={titleClassName}> | ||
| {data.length ? ( | ||
| <PrometheusGraphLink query={query}> | ||
| <PrometheusGraphLink query={isLink ? query : undefined}> | ||
| {data.map((datum, index) => ( | ||
| <React.Fragment key={index}> | ||
| <div className="graph-bar__label"> | ||
|
|
@@ -80,16 +81,17 @@ export const BarChart: React.FC<BarChartProps> = ({ | |
| }; | ||
|
|
||
| export const Bar: React.FC<BarProps> = ({ | ||
| barSpacing, | ||
| barWidth, | ||
| delay = undefined, | ||
| humanize = humanizeNumber, | ||
| isLink = true, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| LabelComponent, | ||
| metric, | ||
| namespace, | ||
| barSpacing, | ||
| barWidth, | ||
| theme, | ||
| query, | ||
| theme, | ||
| title, | ||
| LabelComponent, | ||
| }) => { | ||
| const [response, , loading] = usePrometheusPoll({ | ||
| delay, | ||
|
|
@@ -101,14 +103,15 @@ export const Bar: React.FC<BarProps> = ({ | |
|
|
||
| return ( | ||
| <BarChart | ||
| title={title} | ||
| query={query} | ||
| data={data} | ||
| barSpacing={barSpacing} | ||
| barWidth={barWidth} | ||
| theme={theme} | ||
| loading={loading} | ||
| data={data} | ||
| isLink={isLink} | ||
| LabelComponent={LabelComponent} | ||
| loading={loading} | ||
| query={query} | ||
| theme={theme} | ||
| title={title} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
@@ -119,25 +122,27 @@ type LabelComponentProps = { | |
| }; | ||
|
|
||
| type BarChartProps = { | ||
| LabelComponent?: React.ComponentType<LabelComponentProps>; | ||
| barSpacing?: number; | ||
| barWidth?: number; | ||
| data?: DataPoint[]; | ||
| isLink?: boolean; | ||
| LabelComponent?: React.ComponentType<LabelComponentProps>; | ||
| loading?: boolean; | ||
| query?: string; | ||
| theme?: any; // TODO figure out the best way to import VictoryThemeDefinition | ||
| title?: string; | ||
| data?: DataPoint[]; | ||
| titleClassName?: string; | ||
| loading?: boolean; | ||
| }; | ||
|
|
||
| type BarProps = { | ||
| LabelComponent?: React.ComponentType<LabelComponentProps>; | ||
| barSpacing?: number; | ||
| barWidth?: number; | ||
| delay?: number; | ||
| humanize?: Humanize; | ||
| isLink?: boolean; | ||
| LabelComponent?: React.ComponentType<LabelComponentProps>; | ||
| metric: string; | ||
| namespace?: string; | ||
| barSpacing?: number; | ||
| barWidth?: number; | ||
| query: string; | ||
| theme?: any; // TODO figure out the best way to import VictoryThemeDefinition | ||
| title?: string; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Booleans that default to true tend to be confusing. I'd recommend using
noLinkand flipping the logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Addressed by #4494