feat: add PatternColor for Highcharts pattern fills - #9772
Conversation
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 4 comments.
| Finding | |
|---|---|
Pattern.setColor/setBackgroundColor accept any Color, but only SolidColor serializes to a value Highcharts can use. |
|
| 🧹 | IT helpers query all elements then read each attribute in a Java stream, the N+1 round-trip pattern CONVENTIONS.md forbids. |
| 👀 | IT asserts on the connector's private client-side field __patternSheet, coupling the test to another repo's implementation detail. |
| 🧹 | New Pattern property JavaDocs do not state their default values. |
| * @param color | ||
| * the pattern color | ||
| */ | ||
| public void setColor(Color color) { |
There was a problem hiding this comment.
Pattern.setColor/setBackgroundColor accept any Color, but only SolidColor serializes to a value Highcharts can use.
A caller writes pattern.getPattern().setColor(GradientColor.createLinear(...)) — the parameter type is Color, so this compiles. Serialization then emits "color":{"linearGradient":...,"stops":...} inside the pattern, which Highcharts' pattern.color cannot parse, so the pattern renders with no/black color and the developer gets no error.
The javadoc warns "use a SolidColor", but the type still invites the mistake. The sibling API GradientColor.Stop and addColorStop(double, SolidColor) already type this constraint as SolidColor. Narrow both setColor and setBackgroundColor (line 206) to SolidColor so invalid input fails at compile time.
PatternColor.java:171 · correctness · plausible
There was a problem hiding this comment.
Fixed in 6ca323b. setColor, setBackgroundColor, and the createPath(String, Color, int, int) factory are now typed SolidColor (fields and getters narrowed too), matching GradientColor.addColorStop(double, SolidColor) — invalid input now fails at compile time.
| } | ||
|
|
||
| private List<String> getPatternIds(ChartElement chart, String prefix) { | ||
| return chart.$("pattern").all().stream() |
There was a problem hiding this comment.
🧹 IT helpers query all elements then read each attribute in a Java stream, the N+1 round-trip pattern CONVENTIONS.md forbids.
getPatternIds (line 184) does chart.$("pattern").all() then .map(el -> el.getDomAttribute("id")), and getPointFillAttributes (line 191) does the same for fill. Each per-element getDomAttribute is a separate WebDriver round-trip, and getPatternIds runs on every waitUntil poll (line 164) plus several assertions, multiplying to dozens of extra round-trips per run on a slow CI grid.
CONVENTIONS.md: "Do not simply query all elements and do the filtering from Java as that results in N+1 WebDriver round-trips when accessing each element's data ... prefer a single executeScript." Both helpers are one-line executeScript candidates that return the id/fill list in a single call.
ColumnPatternFillIT.java:184 · conventions · confirmed
There was a problem hiding this comment.
Fixed in 6ca323b. getPatternIds and getPointFillAttributes are now a single executeScript each, returning the list in one round-trip (id/prefix embedded in the script), per CONVENTIONS.md.
|
|
||
| // The reworked CSS-rule mechanism is active: the mixin injects a | ||
| // constructable stylesheet into the shadow root's adoptedStyleSheets. | ||
| Assert.assertTrue("Expected an injected __patternSheet in styled mode", |
There was a problem hiding this comment.
👀 IT asserts on the connector's private client-side field __patternSheet, coupling the test to another repo's implementation detail.
isPatternStylesheetInjected (line 219) checks el.__patternSheet and el.shadowRoot.adoptedStyleSheets, asserted at lines 41 and 107. That field is an internal of the CSS-rule bridge that lives in the web-components dependency, not in this PR's diff. Renaming it or changing how the bridge injects styles breaks this test even though PatternColor's public contract is unchanged.
The public server-side contract (JSON shape) is already fully covered by PatternColorSerializationTest. Consider keeping the IT to the observable outcome — that a patterned point renders a non-solid pattern fill — rather than asserting on the private __patternSheet field name.
ColumnPatternFillIT.java:41 · altitude · plausible
There was a problem hiding this comment.
Fixed in 6ca323b. Removed the __patternSheet coupling entirely; the IT now asserts the observable outcome (patterned points resolve to a vaadin-pattern- fill, and no vaadin-pattern- defs exist in non-styled mode) — no dependency on the web-repo bridge internals.
| * @param width | ||
| * the pattern width | ||
| */ | ||
| public void setWidth(Number width) { |
There was a problem hiding this comment.
🧹 New Pattern property JavaDocs do not state their default values.
None of the Pattern setters (setWidth line 102, setOpacity line 188, setAspectRatio, etc.) document a default. A user reading setOpacity cannot tell whether omitting it sends opacity 1, sends nothing, or errors — every unset field is dropped by NON_NULL serialization, so the Highcharts default applies.
CONVENTIONS.md: "A JavaDoc for a property should state what the default value is." For this new public API, add a short note such as "defaults to unset, so the Highcharts default applies" to each property.
PatternColor.java:102 · conventions · confirmed
There was a problem hiding this comment.
Fixed in 6ca323b. Every Pattern setter JavaDoc now ends with "Defaults to unset, so the Highcharts default applies."
|
2 participants
Adds the Java API for chart pattern fills (an accessibility aid for color-blind users, see #9990). Charts had
SolidColorandGradientColorbut no way to set a pattern fill from Flow.PatternColor(implementsColor, mirrorsGradientColor), withcreatePath(...)andcreateImage(...)factories and a nestedPatternbean exposing the Highcharts pattern options (path/image, width, height, color, opacity, etc.). It serializes to{"pattern":{...}}.PatternColorto theColorjavadoc.Depends on vaadin/web-components#12204
Part of #9990
🤖 Generated with Claude Code