upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

nikhagra-akamai · 2024-11-04T08:14:27Z

Description 📝

Migrated ChartJS to Recharts in CloudPulse.

Note: This PR is raised so that code changes can be reviewed & parallelly we are working on automation test cases which will eventually be added in this PR.

Changes 🔄

List any change relevant to the reviewer.

Replaced LineGraph with AreaChart in CloudPulseLineGraph component.
Updated x-axis ticks & chart variant in CloudPulseWidget
Modified structure the of data passed to AreaChart component in CloudPulseWidgetUtils
Removed CloudPulseColorPalette
Modified legendColor property in metrics display to accept any color
Added a connectNulls property in AreaChart component
Added DataSet type in cloudpulse types

Target release date 🗓️

18th November

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

No Data Display

X-Axis with timestamp ticks for less than 24hrs

X-Axis with date & timestamp ticks for more than 24hrs

Legends rows
https://github.com/user-attachments/assets/bc8886ba-db7a-44ba-aa09-fea7d715bd22

Error State

How to test 🧪

Switch to mock user
Select mandatory filters to generate the graphs
Change time duration filter to see difference in x-axis tick
Modify data in serverHandlers.ts for monitor/services/:serviceType/metrics endpoint to playaround with graph & legends.
You can see a scroll bar in legend rows table if more than 2 legends are present

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-11-04T14:33:04Z

@nikhagra-akamai please address the e2e failures before we get to the review - thx!

hkhalil-akamai · 2024-11-04T22:39:06Z

packages/manager/src/components/LineGraph/MetricsDisplay.tsx

@@ -47,7 +38,7 @@ export interface MetricsDisplayRow {
  data: Metrics;
  format: (n: number) => string;
  handleLegendClick?: () => void;
-  legendColor: LegendColor;
+  legendColor: string;


This looks worse from a type safety perspective. Is this change necessary?

yes it is already been discussed with @jaalah-akamai that instead of restricting to the 7 predefined colors, it should accept any color, so that legend rows greater than 7 won't collide with same color. Also, I don't think color has anything related to typesafety.

hkhalil-akamai · 2024-11-04T22:54:23Z

packages/api-v4/src/cloudpulse/types.ts

+export interface DataSet {
+  [label: string]: number;
+  timestamp: number;
+}


Interfaces in the API SDK should correspond to responses from a particular endpoint. This definition may be more appropriately located in CloudPulseWidgetUtils.ts.

updated, thanks

hkhalil-akamai · 2024-11-04T22:55:47Z

packages/manager/src/components/AreaChart/AreaChart.tsx

+  /**
+   * connect nulls value between two data points
+   */
+  connectNulls? :boolean;


Suggested change

connectNulls? :boolean;

connectNulls?: boolean;

updated, it was there in local changes but forgot to push. Thanks

hkhalil-akamai · 2024-11-04T23:07:25Z

packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx

-          legendRows={legendRows}
+          connectNulls
+          fillOpacity={0.5}
+          legendHeight={theme.spacing(18.75)}


IMO, the overflow in the legend is worse from a usability perspective -- I would prefer to be able to see all rows without a scroll bar.

This is also already been discussed. I understand your concern, but think for dynamically growing legend rows where number can cross 5. Bigger legend row table won't be look so good for UI and if two side by side graphs have different number of legend row count, then that mismatch in graph & legend height will make it look more worse. As in our case legends rows are dynamic & they can grow to 20.

Understood, thanks for the context.

In the latest revision, only two rows fit into the viewport at a time. Do we have any estimates on how many rows were expecting on average? If there are 5+ rows, why are we restricting the window to only show 2 instead of 4 or more?

…eGraph

github-actions · 2024-11-05T07:31:57Z

Coverage Report: ✅
Base Coverage: 86.88%
Current Coverage: 86.9%

nikhagra-akamai · 2024-11-05T09:31:36Z

@nikhagra-akamai please address the e2e failures before we get to the review - thx!

@abailly-akamai This PR is raised so that code changes can be reviewed & parallelly we are working on automation test cases, which will eventually be added in this PR. Please let me know if automation is necessary for the review, I'll make it draft & open once automations are done. - Thx

abailly-akamai · 2024-11-05T15:14:36Z

@nikhagra-akamai I am unsure what you meant, but this just can't be merged with failing tests no matter what cause everyone would inherit these failures

nikhagra-akamai · 2024-11-05T15:25:17Z

@nikhagra-akamai I am unsure what you meant, but this just can't be merged with failing tests no matter what cause everyone would inherit these failures

I only meant that failure tests should not be blocking the PR review. By the time tests are being fixed ( that will be done within a day or 2 ), we can have review for our rest of the PR code.

abailly-akamai · 2024-11-05T15:33:31Z

@nikhagra-akamai fixing the tests may affect the current code and we are trying to minimize the back and forth.

nikhagra-akamai · 2024-11-19T10:10:42Z

we now display the time in the legend, which makes it unreadable on mobile

thank you for highlighting. I've updated

Screen.Recording.2024-11-19.at.4.27.15.PM.mov

… size

…ration

abailly-akamai

Thanks for strengthening the types )it will go a long way!) and addressing the mobile defects. This migration is important, appreciate the tedious work pushing this forward, charts are never easy 👍

Approving pending some extra cleanup on the data types.

abailly-akamai · 2024-11-19T16:37:51Z

packages/api-v4/src/cloudpulse/types.ts

@@ -39,7 +39,7 @@ export interface Widgets {
  namespace_id: number;
  color: string;
  size: number;
-  chart_type: string;
+  chart_type: 'line' | 'area';


nikhagra-akamai · 2024-11-19T16:58:11Z

Thanks @abailly-akamai for the approval

hkhalil-akamai

Thanks for responding to feedback!

…ration

nikhagra-akamai · 2024-11-20T09:40:08Z

thanks @abailly-akamai @hkhalil-akamai for the approvals

couple of updates from the last few comments

Renamed the filename MetricDisplay.styles.ts to MetricsDisplay.styles.ts
Added a DataSet interface in AreaChart component & used the same for type safety in generate12HoursTick function
Added more detailed documentation for xAxisTickCount property

if all good, lets merge the PR

…ration

linode-gh-bot · 2024-11-20T12:11:08Z

Cloud Manager UI test results

🎉 453 passing tests on test run #23 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`0 Failing`	`453 Passing`	`2 Skipped`	`94m 24s`

cypress · 2024-11-20T17:13:33Z

Cloud Manager E2E Run #6855

Run Properties: Passed #6855 • 6c15fbd1c7: upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse (#11204)

Project	`Cloud Manager E2E`
Branch Review	`develop`
Run status	`Passed #6855`
Run duration	`26m 58s`
Commit	`6c15fbd1c7: upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse (#11204)`
Committer	`Nikhil Agrawal`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`2`
Skipped	`0`
Passing	`453`
View all changes introduced in this branch ↗︎

nikhagra-akamai added 12 commits November 4, 2024 13:27

upcoming: [DI-20870] - Migrated chartjs to recharts for ACLP graphs

7be47af

upcoming: [DI-20870] - Removed unused imports

c1d1808

upcoming: [DI-20870] - Removed CloudPulseWidgetColorPalette file

aa9931a

upcoming: [DI-20870] - Optimized code

48fb7bb

upcoming: [DI-21200] - Added connectNulls & color properties

8482608

upcoming: [DI-21200] - Added sort data based on timestamp

801ae23

upcoming: [DI-21200] - Updated x-axis tick format

3fc7ba9

upcoming: [DI-21200] - Updated PR comments

ef29f34

upcoming: [DI-21200] - Hide legends if not data present

1fda168

upcoming: [DI-21200] - Updated legends height

b1b5248

upcoming: [DI-21200] - Updated legend row color type

144818a

upcoming: [DI-21200] - Code refactor

62a5f98

nikhagra-akamai force-pushed the recharts_migration branch from 481e709 to 62a5f98 Compare November 4, 2024 09:39

upcoming: [DI-21200] - Added changeset

a979311

nikhagra-akamai self-assigned this Nov 4, 2024

nikhagra-akamai added the CloudPulse label Nov 4, 2024

nikhagra-akamai marked this pull request as ready for review November 4, 2024 14:22

nikhagra-akamai requested a review from a team as a code owner November 4, 2024 14:22

nikhagra-akamai requested review from hkhalil-akamai and abailly-akamai and removed request for a team November 4, 2024 14:22

hkhalil-akamai reviewed Nov 4, 2024

View reviewed changes

nikhagra-akamai added 2 commits November 5, 2024 12:18

upcoming: [DI-21200] - Moved DataSet type from types to CloudPulseLin…

75adf4d

…eGraph

upcoming: [DI-21200] - Synced with upstream develop

f36786d

nikhagra-akamai closed this Nov 19, 2024

nikhagra-akamai reopened this Nov 19, 2024

nikhagra-akamai added 5 commits November 19, 2024 15:58

upcoming: [DI-21200] - Code optimization

22c7a51

upcoming: [DI-21200] - Updated type safety

9676ce4

upcoming: [DI-21200] - Updated x-axis tick count for different screen…

7a133a7

… size

upcoming: [DI-21200] - updated test cases

d56ed3f

upcoming: [DI-21200] - Fixed eslint error

258d73e

nikhagra-akamai requested review from abailly-akamai, hkhalil-akamai and bnussman-akamai November 19, 2024 12:12

nikhagra-akamai added 3 commits November 19, 2024 18:13

upcoming: [DI-21200] - Added test cases

f0cce9d

upcoming: [DI-21200] - Updated changeset

7db15f4

Merge branch 'develop' of github.com:linode/manager into recharts_mig…

c328d88

…ration

abailly-akamai approved these changes Nov 19, 2024

View reviewed changes

hkhalil-akamai approved these changes Nov 19, 2024

View reviewed changes

hkhalil-akamai added Approved Multiple approvals and ready to merge! and removed Requires Changes labels Nov 19, 2024

nikhagra-akamai added 3 commits November 20, 2024 12:20

Merge branch 'develop' of github.com:linode/manager into recharts_mig…

1895b04

…ration

upcoming: [DI-21200] - Renamed metrics display styles file

496d438

upcoming: [DI-21200] - Updated dataset type in place of any

e2f7b7e

nikhagra-akamai added 2 commits November 20, 2024 15:15

upcoming: [DI-21200] - updated test cases

6e7d2b1

Merge branch 'develop' of github.com:linode/manager into recharts_mig…

a3fc951

…ration

jaalah-akamai merged commit 6c15fbd into linode:develop Nov 20, 2024
23 checks passed

This was referenced Dec 5, 2024

Release v1.133.0 - release → staging #11377

Merged

Release v1.133.0 - staging → master #11378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

nikhagra-akamai commented Nov 4, 2024 •

edited

Loading

abailly-akamai commented Nov 4, 2024

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024 •

edited

Loading

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024 •

edited

Loading

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024

hkhalil-akamai Nov 5, 2024

hkhalil-akamai Nov 18, 2024

github-actions bot commented Nov 5, 2024 •

edited

Loading

nikhagra-akamai commented Nov 5, 2024 •

edited

Loading

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 19, 2024 •

edited

Loading

abailly-akamai left a comment

abailly-akamai Nov 19, 2024

nikhagra-akamai commented Nov 19, 2024

hkhalil-akamai left a comment

nikhagra-akamai commented Nov 20, 2024 •

edited

Loading

linode-gh-bot commented Nov 20, 2024

cypress bot commented Nov 20, 2024

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

Conversation

nikhagra-akamai commented Nov 4, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

As an Author I have considered 🤔

abailly-akamai commented Nov 4, 2024

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2024 • edited Loading

nikhagra-akamai commented Nov 5, 2024 • edited Loading

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 19, 2024 • edited Loading

abailly-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhagra-akamai commented Nov 19, 2024

hkhalil-akamai left a comment

Choose a reason for hiding this comment

nikhagra-akamai commented Nov 20, 2024 • edited Loading

linode-gh-bot commented Nov 20, 2024

Cloud Manager UI test results

🎉 453 passing tests on test run #23 ↗︎

cypress bot commented Nov 20, 2024

nikhagra-akamai commented Nov 4, 2024 •

edited

Loading

nikhagra-akamai Nov 5, 2024 •

edited

Loading

nikhagra-akamai Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading

nikhagra-akamai commented Nov 5, 2024 •

edited

Loading

nikhagra-akamai commented Nov 19, 2024 •

edited

Loading

nikhagra-akamai commented Nov 20, 2024 •

edited

Loading