-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add status property support for series markers #124
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
base: main
Are you sure you want to change the base?
feat: add status property support for series markers #124
Conversation
|
Actually found this change is not accessible. Will work on updating that |
Add support for displaying series markers with different status states, particularly a "warning" status. This enables visual indication of series that require attention or have issues. Changes include: - Extend Highcharts SeriesOptions interface with optional status property - Update ChartSeriesMarker to accept and render status prop - Pass status through chart legend and tooltip components - Add visual examples in marker-permutations page for warning state
d57ee33 to
fab6169
Compare
pages/03-core/core-legend.page.tsx
Outdated
| import pseudoRandom from "../utils/pseudo-random"; | ||
|
|
||
| const i18nStrings = { | ||
| seriesStatusWarningAriaLabel: "warning", |
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.
you can create us a feature request to add support for this prop in the built-in i18n
|
Note that the PR has some conflicts to be solved. |
…icon-to-markers # Conflicts: # src/__tests__/__snapshots__/documenter.test.ts.snap # src/core/chart-api/chart-extra-legend.tsx # src/core/interfaces.ts # src/core/utils.ts
# Conflicts: # src/core/chart-api/chart-extra-legend.tsx # src/core/components/core-tooltip.tsx
Resolved the conflict |
src/core/interfaces.ts
Outdated
| * Specifies the options for each item in the chart. | ||
| * @param id the item id. Can be the id of a series or a point. | ||
| */ | ||
| getItemProps?: (id: string) => ChartItemOptions; |
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.
Two suggestions here:
- Rename to
getItemOptionsto be inline with Highcharts conventions and interface name - Update arguments to
{ itemId: string }type to allow for more options here in the future (name, etc.)
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.
Also, please add ChartItemOptions to the CoreChartProps namespace (see how it is done for other props)
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.
In that scenario, I would have loved to use a pattern I use a lot in Rust:
enum ItemIdentifier {
Series(String)
Point(String)
}
struct GetItemOptionsProps {
item_identifier: ItemIdentifier
}
// then, you could do
fn getItemProps(props: GetItemOptionsProps) -> ChartItemOptions {
match props.item_identifier {
Series(id) => println!("props for series with id {}", id),
Point(id) => println!("props for point with id {}", id),
}
}
Anyway, covered your suggestions in the next rev. Thanks!



Description
Add support for displaying series markers with different status states, particularly a "warning" status. This enables visual indication of series that require attention or have issues.
Changes include:
How has this been tested?
Review checklist
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.