Repository navigation
FIREFLY-2103: Fix reverse axis with limits not working - #2036
Conversation
jaladh-singhal
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A reset with only one bound produces an invalid NaN range while disabling autorange.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes axis reversal with explicit bounds and centralizes Plotly range handling.
Changes:
- Adds shared axis range evaluation utilities.
- Refactors chart options to use them.
- Adds axis direction, bounds, reset, and log-scale tests.
| File | Description |
|---|---|
BasicOptions.jsx |
Delegates axis range changes to the new utility. |
AxisRangeUtil.js |
Implements range, reversal, reset, and log handling. |
AxisRange-test.js |
Covers axis range behavior and regressions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (isLog && (min <= 0 || max <= 0)) return autoscale; // can't show 0 or below on a log scale | ||
| return {[rangeKey]: getRange(min, max, isLog, flip), [autorangeKey]: false}; |
There was a problem hiding this comment.
It's a narrow edge case that pre-dates this PR (the old adjustAxesRange built the same [min, NaN] range hence the test codified it to assess the success of refactor). This only happens when one bound is set with no current range to fill the other end from, as in:
- the axis column is changed and a single bound typed in the same Apply (although changing the column already clears both bounds)
- a new chart is created with one bound
Both of these are extremely rare from user POV since it would require user to set one bound before seeing the new plotted data range.
Still, I fixed this in the latest commit since it's a harmless fix: it now falls back to autoscale in the requested direction instead of Plotly's arbitrary span (e.g. 69.05 to 690.5, x10 guess that plotly does)
This looks good to me. I successfully managed to tailor the axes for my uploaded catalogue.
|
loitly
left a comment
loitly
left a comment
There was a problem hiding this comment.
Tested. The bug is fixed, and the refactored code is clean and easy to understand. Good to go.
FIREFLY-2103: fix reversing chart axis when min/max bounds are set adjustAxesRange rebuilt the range from the user's bounds but took the axis direction from the old layout range, discarding the reverse/un-reverse just requested in the options dialog. Take the direction from the reverse option's changes (re-ordered range or autorange), falling back to the current layout only when the fields have no reverse option. Also simplify the range re-ordering and drop lodash reverse. FIREFLY-2103: centralize chart axis range evaluation Replace the two-pass axis range logic in evalChangesFromFields (reverse option reordering the range, then adjustAxesRange inferring direction back from it) with evalAxisRangeChanges in AxisRangeUtil.js, which decides range and autorange per axis directly from the requested reverse/log options, bounds, column-change reset and current range. Behavior is unchanged except: staying on log no longer rebuilds the range and turns off autoscale on every Apply, and switching to log with a current range reaching 0 or below falls back to autorange instead of a NaN range. Add AxisRange-test.js covering reverse, bounds, column change, log scale and callers without axis options. FIREFLY-2103: autoscale when a single axis bound has nothing to fill the other from With one Y/X bound and no current range to fill the other end from (axis column changed in the same Apply, or a new chart), the range was built with a NaN end, which Plotly drew as an arbitrary span (e.g. 69.05 to 690.5). Fall back to autoscale in the requested direction instead.
2a496e5 to
4bd7230
Compare
Fixes FIREFLY-2103
With Y (or X) Min/Max set in Chart Options, checking or unchecking "reverse" did nothing: the bounds were re-applied using the old layout range's direction instead of the requested one. See the first commit's diff for the fix.
The root issue: the
__xoptions/__yoptionsbranch ofevalChangesFromFieldsencoded the requested direction into a re-ordered range or autorange, andadjustAxesRangeinferred it back. The second commit (most of the diff, separately revertable) replaces both withevalAxisRangeChangesinAxisRangeUtil.js, which decides the range from the dialog inputs directly. This also fixed a log axis losing autoscale on every Apply and a NaN range when switching to log with values ≤ 0.Tests in
AxisRange-test.jswhich were run both before and after the refactor.Testing
https://firefly-2103-reverse-axis-bug.irsakubedev.ipac.caltech.edu/firefly/