Skip to content

Conversation

@psankhe28
Copy link

In visualization: Bar, Line and Pie Chart.

For sidebar, added submenus.

@vercel
Copy link

vercel bot commented Aug 28, 2023

@psankhe28 is attempting to deploy a commit to the UCI Team on Vercel.

A member of the Team first needs to authorize it.

import React, { useEffect, useRef } from "react";
import Chart from "chart.js/auto";
import useWindowSize from "../../hooks/useWindow";

Copy link
Contributor

Choose a reason for hiding this comment

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

can you move this function to some util function

Copy link
Author

Choose a reason for hiding this comment

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

It's a hook so I have kept it in the hooks folder. Should I change?

const { height } = useWindowSize();

useEffect(() => {
// Generate dynamic background colors for each segment based on labels
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's reading data from props,so there are chances that when you reload or during initiation time data may be undefined which will break the condition data.labels.map and return error.So I suggest you to use optional chaining like data?.labels?.map

Copy link
Author

Choose a reason for hiding this comment

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

I have updated it in the recent push.

data: {
labels: data.labels,
datasets: [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

here also add optional chaining

Copy link
Author

Choose a reason for hiding this comment

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

I have updated it in the recent push.


return (
<div
style={{
Copy link
Contributor

Choose a reason for hiding this comment

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

move inline styles to a style class

Copy link
Author

Choose a reason for hiding this comment

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

I have updated it in the recent push.

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.

2 participants