-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
feat: personal flag metrics display #8232
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
} | ||
|
||
export const FEATURE_METRIC_HOURS_BACK_DEFAULT = 48; | ||
|
||
export const FeatureMetricsHours = ({ | ||
hoursBack, | ||
setHoursBack, | ||
label = <StyledTitle>Period</StyledTitle>, |
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.
allowing to disable the label by passing undefined. In the feature metrics the label is needed but in personal dashboard it's not.
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.
As long as this doesn't cause any a11y issues, I have no problems. But I notice that the general select component doesn't have a label associated with it. Input elements should always be properly labeled (with label elements tied to the component). You can hide the label visually if you want, but it should def be there for assistive tech.
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.
Of course, the input's been around for a while, probably, but we can still fix it up now that we're here 💁🏼
return `${value} (${percentage}%) - ${key}`; | ||
}; | ||
|
||
export const createPlaceholderBarChartOptions = ( |
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.
moved from a separate file
hoursBack: number, | ||
locationSettings: ILocationSettings, | ||
): ChartOptions<'bar'> => { | ||
const { plugins, responsive, elements, interaction, scales } = |
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.
reuse options that we share with the placeholder settings
return { | ||
plugins: { | ||
legend: plugins?.legend, | ||
tooltip: { |
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.
tooltip only shows up when we have metrics data, it's not present in the placeholder. The styles are copied from the NetworkTrafficUsage. We may eventually create a shared tooltip styling after we get another use case (avoid premature abstraction)
usePointStyle: true, | ||
caretSize: 0, | ||
boxPadding: 10, | ||
callbacks: { |
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.
all the callbacks are copied from the feature metrics line chart
ticks: { | ||
color: theme.palette.text.secondary, | ||
callback(tickValue) { | ||
const label = this.getLabelForValue(Number(tickValue)); |
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.
this was tricky to find. chartjs doesn't return timestamps in the callback but indices. So I lookup the actual value using a utility function provided in the latest version of chartjs
return { environment, setEnvironment, activeEnvironments }; | ||
}; | ||
|
||
const useFlagMetrics = ( |
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.
copied all this code from the feature metrics. One difference is that now we have a hook that prepares chart options + chart data in one function instead of going through multiple components
@@ -0,0 +1,42 @@ | |||
import type { IFeatureMetricsRaw } from 'interfaces/featureToggle'; |
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.
this is a copy paste of the feature metrics with colors swapped and total series removed
} | ||
|
||
export const FEATURE_METRIC_HOURS_BACK_DEFAULT = 48; | ||
|
||
export const FeatureMetricsHours = ({ | ||
hoursBack, | ||
setHoursBack, | ||
label = <StyledTitle>Period</StyledTitle>, |
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.
As long as this doesn't cause any a11y issues, I have no problems. But I notice that the general select component doesn't have a label associated with it. Input elements should always be properly labeled (with label elements tied to the component). You can hide the label visually if you want, but it should def be there for assistive tech.
} | ||
|
||
export const FEATURE_METRIC_HOURS_BACK_DEFAULT = 48; | ||
|
||
export const FeatureMetricsHours = ({ | ||
hoursBack, | ||
setHoursBack, | ||
label = <StyledTitle>Period</StyledTitle>, |
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.
Of course, the input's been around for a while, probably, but we can still fix it up now that we're here 💁🏼
About the changes
Details:
Next steps:
Important files
Discussion points