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

feat: ✨ Add danger and safe zone to charts #111

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Conversation

AntoineThibi
Copy link
Contributor

image image

@netlify
Copy link

netlify bot commented May 26, 2023

Deploy Preview for flashlight-docs ready!

Name Link
🔨 Latest commit d7d8bda
🔍 Latest deploy log https://app.netlify.com/sites/flashlight-docs/deploys/6476fd3ce90006000854d615
😎 Deploy Preview https://deploy-preview-111--flashlight-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@AntoineThibi AntoineThibi requested a review from Almouro May 26, 2023 07:38
@flashlight-bot
Copy link
Contributor

Flashlight Automated Report 🔦

@@ -61,6 +61,9 @@ export const CPUReport = ({
height={500}
interval={POLLING_INTERVAL}
series={totalCPUUsage}
annotationIntervalList={[
{ y: 180, y2: 1000, color: "red", label: "Danger Zone" },
Copy link
Member

Choose a reason for hiding this comment

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

can we make those constants? to make sure the big useMemo containing the options doesn't rerender

Also color wise, can we use:
error: "#E62E2E",
success: "#158000"

We can hardcode tbh if we don't have a fast way to reuse the theme

@@ -107,6 +137,7 @@ export const Chart = ({
},
annotations: {
xaxis: videoEnabled ? [getVideoCurrentTimeAnnotation()] : [],
yaxis: getAnnotationInterval(annotationIntervalList),
Copy link
Member

Choose a reason for hiding this comment

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

eslint is saying you need to add the annotationIntervalList to the array of deps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The eslint config and prettier is a little weird isn't it ? It does the prettier only after the push...

@@ -72,6 +100,7 @@ export const Chart = ({
maxValue,
showLegendForSingleSeries,
colors = getColorPalette(),
annotationIntervalList = [],
Copy link
Member

Choose a reason for hiding this comment

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

same here, make it null by default to avoid recomputing the option below

@flashlight-bot
Copy link
Contributor

Flashlight Automated Report 🔦

@flashlight-bot
Copy link
Contributor

Flashlight Automated Report 🔦

@Almouro Almouro merged commit da0d803 into main Jun 2, 2023
@Almouro Almouro deleted the Chart-Add-Zone branch June 2, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants