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

Remove chart component #4518

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Remove chart component #4518

merged 1 commit into from
Jan 7, 2025

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Jan 6, 2025

What

Removes the chart component.

Code removal based on changes added in #4301 and #4312

Slightly depends on alphagov/frontend#4562 (which is the only place this component is used) but since none of the instances of this are actually used should still be safe to merge and deploy.

Why

The component is no longer required. It was experimental and not fully tested with users, so if we have a requirement for charts again in the future it would be better to build a new component based on further research.

Visual Changes

No more charts...

Screenshot 2025-01-06 at 15 58 31

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4518 January 6, 2025 09:42 Inactive
@andysellick andysellick marked this pull request as ready for review January 6, 2025 13:12
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4518 January 6, 2025 13:13 Inactive
@andysellick
Copy link
Contributor Author

@maxgds potentially controversial PR, any thoughts?

@maxgds
Copy link
Contributor

maxgds commented Jan 6, 2025

@maxgds potentially controversial PR, any thoughts?

Well we need to either retire this chart component or retire the one in Content Data, and one of them is actually in use so... That said, it would be good to shift the improvements to the in-use version, though probably not all the extra options. And it's questionable whether that should be the web group's job.

@andysellick
Copy link
Contributor Author

Well we need to either retire this chart component or retire the one in Content Data, and one of them is actually in use so... That said, it would be good to shift the improvements to the in-use version, though probably not all the extra options. And it's questionable whether that should be the web group's job.

Agreed. I've posted a message in slack for the platform engineering team about this change and will leave it to them to decide if they want to migrate any of the changes we've made, which will still be visible to them after this PR is done with.

@andysellick
Copy link
Contributor Author

Update - the team aren't doing any improvement work in content-data-admin, so don't need any of the changes we've made.

Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Code removal looks ok to me 👍

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4518 January 7, 2025 09:21 Inactive
@andysellick andysellick merged commit bbcdb24 into main Jan 7, 2025
12 checks passed
@andysellick andysellick deleted the remove-chart branch January 7, 2025 09:27
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.

4 participants