fix: Fix missing and incorrect copyWith parameters across chart tooltip data classes#2094
fix: Fix missing and incorrect copyWith parameters across chart tooltip data classes#2094WandsonDev wants to merge 1 commit into
Conversation
…ip data classes Several copyWith methods were incomplete or had bugs that silently ignored caller-supplied values, making it impossible to update chart configuration through copyWith. - BarChartRodData: rename parameter `dashArray` → `borderDashArray` and add the missing `?? this.borderDashArray` fallback so the parameter is actually forwarded instead of silently discarded - BarTouchTooltipData: add missing copyWith method; also add `direction` field to props so equality checks reflect direction changes - LineTouchTooltipData: add missing copyWith method - CandlestickTouchTooltipData.copyWith: add missing `showOnTopOfTheChartBoxArea` parameter - ScatterTouchTooltipData.copyWith: add missing `tooltipBorderRadius` parameter - ScatterChartData.props: remove duplicate `rangeAnnotations` entry Regression tests added for each fix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2094 +/- ##
=======================================
Coverage 93.07% 93.07%
=======================================
Files 50 50
Lines 3956 3956
=======================================
Hits 3682 3682
Misses 274 274
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Gradient? gradient, | ||
| double? width, | ||
| BorderRadius? borderRadius, | ||
| List<int>? dashArray, |
There was a problem hiding this comment.
The only thing that worries me is this property that we're removing (it adds a breaking change)
But the point is that the value here was ignored. So I don't know what to do, haha.
Maybe we can go ahead with the breaking change as it is a special edge-case
There was a problem hiding this comment.
In a professional way, we should deprecate the previous name and introduce the new name. But in this case it doesn't really make sense.
|
Please let me know what you think, other than that, it looks good to me. |
2 participants
Description
Several
copyWithmethods across chart data classes were incomplete or had bugs that silently ignored caller-supplied values, making it impossible to update chart configuration throughcopyWith.Changes
BarChartRodData.copyWith— The parameter was nameddashArrayinstead ofborderDashArray, causing the constructor call to resolve tothis.borderDashArray(the instance field) instead of the caller-supplied value. Renamed toborderDashArrayand added the missing?? this.borderDashArrayfallback.BarTouchTooltipData— Added missingcopyWithmethod. Also added thedirectionfield toprops, which was missing from the equality check — two objects with differentdirectionvalues were incorrectly considered equal.LineTouchTooltipData— Added missingcopyWithmethod.CandlestickTouchTooltipData.copyWith— Added missingshowOnTopOfTheChartBoxAreaparameter.ScatterTouchTooltipData.copyWith— Added missingtooltipBorderRadiusparameter.ScatterChartData.props— Removed duplicaterangeAnnotationsentry.Tests
Regression tests added for each fix to prevent regressions.
Checklist
make surepasses (tests + checkstyle)BarChartRodData.copyWithaligns with the existing field name — the olddashArrayname was never usable due to the bug)