-
Notifications
You must be signed in to change notification settings - Fork 20
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
Remove chart component #4518
Conversation
312011e
to
955dd10
Compare
@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. |
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. |
Update - the team aren't doing any improvement work in |
955dd10
to
5bf812b
Compare
5bf812b
to
5a29e2e
Compare
There was a problem hiding this 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 👍
5a29e2e
to
533a464
Compare
533a464
to
ca1c540
Compare
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...