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

upcoming: [M3-7301] - Replace Linode Network Transfer History chart with Recharts #9938

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Nov 27, 2023

Description 📝

This PR is part of a bigger effort to replace chart.js with the more modern Recharts. As a first step, replace the Linode Network Transfer History chart.

Preview 📷

Before After
before.mov
Screen.Recording.2023-11-29.at.4.34.17.PM.mov

How to test 🧪

Prerequisites

  • Have a Linode that's been running for a while

Verification steps

  • Go to the Network tab of a Linode that's been running for a while
  • Confirm the updated UI and that there are no regressions in the graph, units, etc

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@hana-akamai hana-akamai added the Recharts Effort to replace chartjs library label Nov 27, 2023
@hana-akamai hana-akamai self-assigned this Nov 27, 2023
@hana-akamai hana-akamai changed the title feat: Replace Linode Network Transfer History chart with Recharts upcoming: Replace Linode Network Transfer History chart with Recharts Nov 27, 2023
@hana-akamai hana-akamai changed the title upcoming: Replace Linode Network Transfer History chart with Recharts upcoming: [M3-7301] - Replace Linode Network Transfer History chart with Recharts Nov 28, 2023
@hana-akamai hana-akamai marked this pull request as ready for review November 28, 2023 20:14
@hana-akamai hana-akamai requested a review from a team as a code owner November 28, 2023 20:14
@hana-akamai hana-akamai requested review from carrillo-erik, coliu-akamai, jdamore-linode and bnussman-akamai and removed request for a team November 28, 2023 20:14
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

This is awesome. I am very eager to get Chart.js out of here 🗑️

Can we improve the dark mode experience a bit?

Screenshot 2023-11-29 at 10 50 45 AM

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Nice work @hana-linode, this is a nice improvement over Chart.js!

Functionality wise I don't see any issues; I played around with it using Chrome and Firefox (and tested using VoiceOver on both) and didn't catch any issues.

Besides the dark theme issues Banks pointed out, the only definite accessibility issue I’m seeing is the green "Public Outbound Traffic" text on the tooltip. In Prod this is black and bold -- the green doesn't pass WebAIM's contrast checker.

Screenshot 2023-11-29 at 9 50 34 AM
Screenshot 2023-11-29 at 10 50 14 AM

The only other thing that might be worth raising attention to is the animations. They look nice but are also kind of slow and easily triggered (by clicking the prev and next buttons, and by resizing the window, etc.). I'm wondering if it would be worthwhile to disable the animations using the prefers-reduced-motion media query (I'm not aware of anywhere else where we do this in Cloud, but I think it would be a nice thing to do).

@bnussman-akamai
Copy link
Member

Dark mode looks much better now, but would it be possible for the tooltip to have a dark gray background with white text instead of a while background? I think that would look more fitting with our theme.

Hopefully we can do this without adding a ton of extra code. If we need to, we can define a custom component to use as the tooltip (https://github.com/bnussman/managernext/blob/main/src/linodes/Stats.tsx#L121-L132)

@carrillo-erik
Copy link
Contributor

Nice, I really like the refreshed look of the chart. I tested the changes in mobile view and only noticed a small visual regression, however, it appears that the dates displayed to the user are also different.

OLD NEW
bb22 aa11

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Looks good @hana-linode!

@hana-akamai
Copy link
Contributor Author

hana-akamai commented Dec 1, 2023

@carrillo-erik Updated x-axis intervals. Getting the dates to be exact is more complicated; I don't think it matters that they are the same bc the information is still correct, just a different date is displayed

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

This is awesome!

I'd love to see the NodeBalancer graphs done next!

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

✅ confirmed no regressions in light mode
✅ confirmed no regressions in dark mode
✅ confirmed no regressions on other browsers (chrome, safari, firefox)

great work Hana!

@coliu-akamai coliu-akamai added the Approved Multiple approvals and ready to merge! label Dec 1, 2023
@hana-akamai hana-akamai merged commit 9eee3aa into linode:develop Dec 1, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Recharts Effort to replace chartjs library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants