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

[v0.5] - Series API change #45

Open
wants to merge 2 commits into
base: v0.5-dev
Choose a base branch
from
Open

[v0.5] - Series API change #45

wants to merge 2 commits into from

Conversation

jentfoo
Copy link
Member

@jentfoo jentfoo commented Feb 8, 2025

For ease of adoption, it's important that chart options are obvious and functional. Having extra fields in the struct which only work under certain chart types or conditions can make it difficult to adopt our library. Series is a notable exception of this design pattern as it contains:

  • MarkLine & MarkPoint - currently only supported on Line and Bar charts (Horizontal Bar charts will be a future feature expansion)
  • Radius - currently only supported on Pie charts and Radar charts (partially, Radar charts do not support per-series radius differences)

In order to make the Series struct able to be flexible to match exactly the chart type, this change creates a XxxSeries struct for each chart type. This has the added benefit of removing the Type field, since the type is defined through the Series struct itself.

Negatives:

  • This introduces a lot of code in series.go, with many small but often mostly duplicate implementations to just handle the different types
  • Use of ChartOptions is slightly worse - Since this struct is generic for all chart types the former Series struct was a nice fit. We introduced GeneralSeries which maintains the flexibility the old struct had. It needs to be constructed through NewSeriesListGeneral or through the former methods and then invoking .ToGeneralSeries() in order to convert the chart specific series to the general one.

Alternatives considered:

These chart configuration options could be moved from the Series struct into the specific Chart Option struct. This provides an easy way to discover these options (vs being multiple structs deep). It also allows us to create MarkLines and other features that don't have to be correlated to a specific series (e.g. our Global feature for MarkLine and MarkPoint's).

The challenge with this is that in most cases these fields need to be correlated to a specific series. Which introduces:

  • The need to reference the series by index (which could change depending on the order of the Legend names)
  • Or the need to reference by the series name (not often set)

If we did adopt this we could easily add an SeriesIndex field in subsequent structs (e.g. SeriesMarkLine), but this would require this additional field to be manually set.

We could alternatively use a map with the key being the series index, this would force a clear choice but will require the Options struct map's to be initialized.


Overall I want to consider this for v0.5 because I believe it produces an external API that is easier to understand and adopt for new users, with the primary burden being internal complexity. However I am opening this as second PR so I can get some specific review and consideration around this very significant API change.

@jentfoo jentfoo added the enhancement New feature or request label Feb 8, 2025
@jentfoo jentfoo self-assigned this Feb 8, 2025
@jentfoo jentfoo force-pushed the v0.5-series_change branch from 37d1cfa to 660dd62 Compare February 8, 2025 02:48
@jentfoo jentfoo force-pushed the v0.5-series_change branch 2 times, most recently from 205fc03 to 31980b4 Compare February 10, 2025 05:45
This change makes the `Series` contain only the fields used by all chart types. MarkPoint, MarkLine, and Radius have moved to Series types specific to the charts that support them.

This helps make the API more obvious, by knowing that available fields are all potentially useful if desired. It also will make future changes easier as we expand support for series specific marks and adjustements.
This results in a bit of an increase in code and field duplication. However this dramatically improves the experience of constructing the struct series manually if a user chooses to do it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant