feat(plugin-chart-echarts): allow decal patterns in custom ECharts options - #43427
feat(plugin-chart-echarts): allow decal patterns in custom ECharts options#43427rlei-odes wants to merge 1 commit into
Conversation
…tions itemStyle.decal is how ECharts expresses a patterned fill — hatching rather than a solid colour. It was the only member of itemStyle missing from the custom-options allowlist, so it was silently stripped, while its siblings borderColor, borderWidth and borderType were accepted. Adds decalSchema, mirroring ECharts' DecalObject, and references it from itemStyleSchema.
dosubot
Bot
added
change:frontend
Code Review Agent Run #cfd8faActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| dashArrayX: z | ||
| .union([z.number(), z.array(z.union([z.number(), z.array(z.number())]))]) | ||
| .optional(), |
There was a problem hiding this comment.
Suggestion: The dashArrayX schema accepts heterogeneous arrays such as [1, [2, 3]] because each outer element independently allows either a number or a number array. ECharts expects this option to be one uniform representation—number, number[], or number[][]—so malformed mixed values pass validation and are forwarded to the renderer. Model the union at the array level to reject mixed nesting. [type error]
Severity Level: Minor 🧹
- ⚠️ Custom ECharts decal validation accepts malformed mixed arrays.
- ⚠️ Timeseries custom options can forward invalid decal geometry.
- ⚠️ ECharts may ignore or mishandle the affected pattern.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts
**Line:** 124:126
**Comment:**
*Type Error: The `dashArrayX` schema accepts heterogeneous arrays such as `[1, [2, 3]]` because each outer element independently allows either a number or a number array. ECharts expects this option to be one uniform representation—`number`, `number[]`, or `number[][]`—so malformed mixed values pass validation and are forwarded to the renderer. Model the union at the array level to reject mixed nesting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The issue is correct. The current To resolve this, update the dashArrayX: z
.union([z.number(), z.array(z.number()), z.array(z.array(z.number()))])
.optional(),This change ensures that superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43427 +/- ##
=======================================
Coverage 78.85% 78.85%
=======================================
Files 2876 2876
Lines 164581 164582 +1
Branches 38011 38011
=======================================
+ Hits 129786 129787 +1
Misses 32348 32348
Partials 2447 2447
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the flag — I checked this against ECharts and I believe the current schema is correct, so I'd like to leave it as is. ECharts types the option as a per-element union rather than a uniform one: // echarts/types/dist/shared.d.ts
declare type DecalDashArrayX = number | (number | number[])[];which is what Mixed nesting is intentional rather than malformed. From /**
* [20, 5] should be normalized into [[20, 5]],
* while [20, [5, 10]] should be normalized into [[20, 20], [5, 10]]
*/The loop below it branches per element — a number becomes a Narrowing to |
1 participant
SUMMARY
itemStyle.decalis the ECharts property for a patterned fill — hatching,cross-hatching, dots — rather than a solid colour. It is the only member of
itemStylemissing fromitemStyleSchemaineChartOptionsSchema.ts, so it isstripped during validation while
color,borderColor,borderWidth,borderType,borderRadius, the shadow properties andopacityall pass.That gap looks accidental. A decal is the same kind of value as its siblings — a
static data structure, which is what the safe parser exists to permit — and it
introduces no new type of input:
symbolreuses thesymbolTypeSchemastringalready accepted for series symbols and legend icons. The schema mirrors
ECharts'
DecalObjectin full rather than a subset, in the same waylineStyleSchemadoes, so there is no arbitrary line to defend later.What this does not do. It does not make patterned fills usable. A decal is a
per-series property, and per-series options cannot be reached through this
control at all today:
mergeCustomEChartOptionsreplaces arrays rather thanmerging them, so supplying
seriesdiscards the computed series, and thereplacement cannot carry its own data because
datais deliberately not inseriesSchema. Closing the allowlist gap is one half of making a decalreachable; the merge behaviour is the other, and that is a larger question than
this PR.
So this is a small consistency fix rather than a feature. I am opening it
because it is a one-property gap that costs nothing, and because I am working
toward the other half — per-series visual treatments where the fill pattern
carries meaning independently of colour, which is a common convention in
business reporting. Background and the wider discussion:
#43426
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — this widens what the options schema accepts and adds no
control. Nothing renders differently unless a chart opts in through the
existing ECharts Options control, which the testing instructions cover.
TESTING INSTRUCTIONS
Unit tests:
npm run test -- plugins/plugin-chart-echarts/src/utils/safeEChartOptionsParserThe unit tests are the verification here, because a decal cannot currently be
exercised end to end through the control, for reasons unrelated to this
change:
series;mergeCustomEChartOptionsreplaces arrays rather than merging them, sosupplying
seriesdiscards the computed series;datais not inseriesSchema— by design, since data comes from the query.So the option is accepted and merged, and nothing in Superset sets one yet.
That is what makes this a prerequisite rather than a feature, and I would rather
say so than dress up a demo.
What is worth checking for regressions:
top-level keys such as
xAxis/yAxis, and confirm it renders as before.Those merge as objects and are unaffected.
{ series: { itemStyle: { decal: { rotation: 'sideways' } } } }raises avalidation error rather than being silently dropped — and that unknown keys
inside a decal are still stripped. Both are covered by the new unit tests.
ADDITIONAL INFORMATION