Skip to content

refactor(charts-ng): filter out empty strings from echarts color palette and avoid warnings#1630

Open
akashsonune wants to merge 1 commit intomainfrom
refactor/fix-echarts-warning
Open

refactor(charts-ng): filter out empty strings from echarts color palette and avoid warnings#1630
akashsonune wants to merge 1 commit intomainfrom
refactor/fix-echarts-warning

Conversation

@akashsonune
Copy link
Copy Markdown
Member

@akashsonune akashsonune commented Mar 12, 2026

image

Echarts does not like empty strings in color array


Documentation.
Examples.
Dashboards Demo.
Playwright report.

Coverage Reports:

Code Coverage

@akashsonune akashsonune changed the title refactor(charts-ng): filter out empty strings from echarts color pale… refactor(charts-ng): filter out empty strings from echarts color palette and avoid warnings Mar 12, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to prevent warnings from echarts by filtering out empty strings from the color palette. While the intention is good, the current implementation introduces a potential runtime error by unconditionally calling .filter() on the palette variable, which is of type any. If palette holds a truthy non-array value (like a single color string), the application will crash. I've added a review comment with a suggestion to add a type guard to handle this case gracefully, making the code more robust.

Comment thread projects/charts-ng/common/si-chart-base.component.ts Outdated
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the chart component to filter out empty strings from the ECharts color palette, which prevents warnings in the console. The change is correct and addresses the issue. I've added one suggestion to improve the robustness of the code by handling cases where the theme palette might not be an array, which would prevent potential runtime errors. I've adjusted the severity to medium.

Comment thread projects/charts-ng/common/si-chart-base.component.ts Outdated
@akashsonune akashsonune force-pushed the refactor/fix-echarts-warning branch from d2a02ff to fa6a250 Compare March 12, 2026 18:45
@akashsonune akashsonune marked this pull request as ready for review March 12, 2026 19:02
@akashsonune akashsonune requested review from a team as code owners March 12, 2026 19:02
@akashsonune akashsonune requested a review from dr-itz March 12, 2026 19:07
@spike-rabbit
Copy link
Copy Markdown
Member

@akashsonune how can those empty strings be part of the palette in the first place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants