Skip to content

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented May 17, 2024

Description 📝

This PR gets rid of the recharts feature flag in order to remove heavy dependencies we're shipping to our users in the Linode detail flow.

  • charts.js
  • moment,js (which is a dpendency of the above)

It cleans up conditional rendering of the old charts since everything has been stable since the new charts have been released.

Only remaining components using charts.js:

  • GaugePercent
  • LineGraph

which are both only for longview 🎉

🥇 Thanks @hana-linode for the super clean handling of splitting the feature initially, was very easy to clean up

Changes 🔄

  • Remove flag and and old charts behind it

Preview 📷

No visual change expected as part of this PR

How to test 🧪

Verification steps

  • Confirm all charts are showing (recharts) in both the Linode detail/networking and managed UIs

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

}

// @todo recharts: delete this file when we decide recharts is stable (new version is AccessibleAreaChart)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't really delete it because LineCharts is till being used in longview (only place after this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we should keep some kind of comment as a reminder to remove this though

@abailly-akamai abailly-akamai marked this pull request as ready for review May 17, 2024 15:04
@abailly-akamai abailly-akamai requested a review from a team as a code owner May 17, 2024 15:04
@abailly-akamai abailly-akamai requested review from hkhalil-akamai and jaalah-akamai and removed request for a team May 17, 2024 15:04
@github-actions
Copy link

github-actions bot commented May 17, 2024

Coverage Report:
Base Coverage: 81.52%
Current Coverage: 81.52%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Great clean up 🧹

}

// @todo recharts: delete this file when we decide recharts is stable (new version is AccessibleAreaChart)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we should keep some kind of comment as a reminder to remove this though

@hana-akamai hana-akamai added the Approved Multiple approvals and ready to merge! label May 17, 2024
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! Tech Debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants