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

ref(dashboards): Move unit scaling into Plottable #86116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Feb 28, 2025

This is a refactor of the Plottable interface. It clarifies and separates the type definitions, and moves some ugly mutation logic away out of the React component and into the constructor classes.

This is an important step on the road to multi-type charts, where lines and bars can coexist. It'll become a lot clearer when the public API for TimeSeriesWidgetVisualization starts accepting plottables directly.

Changes

The first change is a clarification of the types, and their relationships.

  • Plottable is a generic interface. A Plottable is an object that can be put into ECharts. Right now, only one kind of plottable is implemented: PlottableData, but others are coming (e.g., Releases, and later other marker types). Right now, PlottableData is only used under-the-hood in TimeSeriesDataVisualization, but soon this will be the public API. Charts will be created by passing various plottable items to the component
  • Area, Line, and Bars all implement the PlottableData interface. They do this by extending the AggreateTimeSeries plottable. AggregateTimeSeries is there for sharing code. It specifies the constructor, and some simple scaling logic. Soon, this class will have more interesting properties that will make coordinating data between different plottables easier
  • A plottable has a configuration and plotting options. The configuration has to do with the data. How much is that data delayed? Does it have a specific color? The plotting options have to do with putting the plottable into ECharts. This answers questions like "what is the fallback color?" and "what is the Y axis unit"? These things are computed after taking all plottables into consideration

The second change is smaller, and more subtle. It moves data scaling into the plottables. This creates just a bit of code duplication, but is much nicer because this logic is now hidden from the chart (so I can improve it later).

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Feb 28, 2025
Comment on lines +30 to +31
timeSeries: Readonly<TimeSeries>;
config?: Readonly<TConfig>;
Copy link
Member Author

@gggritso gggritso Feb 28, 2025

Choose a reason for hiding this comment

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

timeSeries and config are back and stored on the object! This is needed because in order to decide how to plot all the series together, I need to check their types and units and colours against each other. Plus I need the original data to compute the correct scaling at render time.

@gggritso gggritso requested a review from JonasBa February 28, 2025 18:23
@gggritso gggritso marked this pull request as ready for review February 28, 2025 18:23
@gggritso gggritso requested a review from a team as a code owner February 28, 2025 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant