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

Proposal: Deprecate LineChart, AreaChart, BarChart, ScatterChart in favour of ComposedChart #3380

Open
nikolasrieble opened this issue Feb 19, 2023 · 7 comments
Labels
breaking change Use this label to indicate a breaking change in the Recharts API refactor PRs that refactor existing code

Comments

@nikolasrieble
Copy link
Contributor

nikolasrieble commented Feb 19, 2023

It is not clear what benefit one gains by using specific Cartesian Chart wrappers such as the AreaChart or the LineChart, when instead all charts can be achieved using the ComposedChart.

Proposal:

  • Deprecate LineChart, AreaChart, BarChart, ScatterChart
  • Rename ComposedChart to CartesianChart

Advantages:

  • Reduced API, thus user-facing complexity
  • Reduced bug surface

Disadvantages:

  • ?

Open Questions:

  • The ScatterChart currently has a separate Tooltip behaviour - should the Scatterplot remain?
  • Is the interface of the ComposedChart equal to the union of fields of the LineChart, BarChart and AreaChart? What is missing?
@nikolasrieble nikolasrieble changed the title Proposal: Deprecate all charts but ComposedChart Proposal: Deprecate LineChart, AreaChart, BarChart, ScatterChart in favour of ComposedChart Feb 19, 2023
@nikolasrieble
Copy link
Contributor Author

@proke03 @arcthur @ckifer @Yilun-Sun
I would love to hear your input. This is a refactoring idea, that could simplify Recharts for an upcoming major release.

@nikolasrieble nikolasrieble added breaking change Use this label to indicate a breaking change in the Recharts API refactor PRs that refactor existing code labels Feb 19, 2023
@nikolasrieble nikolasrieble pinned this issue Feb 21, 2023
@ckifer
Copy link
Member

ckifer commented Feb 21, 2023

I think we should do a deep dive on this on your proposed open questions (I think the second one is SUPER important for type safety/type issues).

Lets look at both open and closed issues related to ComposedChart (since a lot of issues have been closed without a solution) and list out what we know. I would also like to do a performance profile - is ComposedChart slower when used with say Line as opposed to LineChart and Line?

This is a big API change and hence one I wouldn't want to make without careful consideration

@nikolasrieble
Copy link
Contributor Author

Before we spend more time here, I would like to have a few more reference opinions. What do you think? @akamfoad @HHongSeungWoo @andrewangelle @PavelVanecek

@ckifer
Copy link
Member

ckifer commented Nov 1, 2023

I have concerns, but if we can solve those I am all for this.

Mostly:

  • Does the current Tooltip behavior act the same when you have 1 composed chart + 1 line/scatter/bar/area respectively? (I think currently not) - this needs addressed.
    • Does the Tooltip cursor behave the same (also think not)
  • We have some high-level props that are specific to each chart type on the {Type}Chart components (e.g. barWidth, barGap). Are those okay to be here? How are they treated if they stay? Do we have all the ones we need?
  • Prevent another generateCategoricalChart situation - even though these are handled "together" in terms of the wrapper component, all specific logic to the sub-components should remain within its own files

@HHongSeungWoo
Copy link
Member

I believe this decision will be easier for us when we remove unnecessary elements and refactor the code sufficiently. While I fully agree with unifying charts with Cartesian coordinates, it seems that there are too many side effects to proceed without substantial refactoring

I am waiting for a store or context for separating the components

@andrewangelle
Copy link
Contributor

The idea to simplify the API is great. One parent chart component coupled with the individual compose-able elements is a better API. Though, I think the decision should put a lot of weight on if it also makes maintenance and updates easier, and simplifies the architecture.

Does the current Tooltip behavior act the same when you have 1 composed chart + 1 line/scatter/bar/area respectively? (I think currently not) - this needs addressed.
Does the Tooltip cursor behave the same (also think not)

Couple of manual tests are showing different tooltip behavior particularly with the cursor

Prevent another generateCategoricalChart situation

+1. That thing is a wild untamed beast 😄

@nikolasrieble
Copy link
Contributor Author

We would have to extend the Tooltip first, to allow switching between the two types of behaviour for all chart types. That is certainly a reasonable feature request either way, many users want to only see the value of a single data point / line / dot for a LineChart - I had to implement workarounds multiple times already.

Vice versa, one might want to see multiple scatter values per xAxis in the ScatterChart.

So I suggest we extract a separate task here to extend the Tooltip, and consider that one blocking for this deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Use this label to indicate a breaking change in the Recharts API refactor PRs that refactor existing code
Projects
Status: No status
Development

No branches or pull requests

4 participants