DM-9343:scatter and line chart zoom changed when changing between "points" and "connected points" #321
Conversation
|
It works great for me. This might be introduced with your change, not sure, i just wanted to mention it to you. Otherwise, please, raise a separate ticket for the reset problem to be looked in the future. |
There was a problem hiding this comment.
I think the right way to fix the issue is to preserve zoom level in XYPlotOptions if x and y columns did not change.
-
Modify resultSuccess in XYPlotOptions to optionally pass current zoom or undefined.
-
Pass xyPlotParams.zoom in onSuccess callback of CompleteButton on line 472 of XYPlotOptions (if x.columnOrExpr and y.columnOrExpr columns did not change from what is in xyPlotParams)
I am not even sure we should bother removing zoom when the new data are fetched.
| @@ -194,7 +194,9 @@ export function setZoom(chartId, chartDataElementId, selection=undefined) { | |||
| if (decimatedUnzoomed || isUndefined(decimatedUnzoomed)) { | |||
| noFetch = false; | |||
| } | |||
| dispatchChartOptionsUpdate({chartId, chartDataElementId, updates: {zoom: xyPlotParams.selection, selection: undefined}, noFetch}); | |||
| dispatchChartOptionsUpdate({chartId, chartDataElementId, updates: {zoom: xyPlotParams.boundaries, selection: undefined}, noFetch}); | |||
There was a problem hiding this comment.
As I said this morning, you should not use boundaries property, it has nothing to do with zoom. Using it messes up setting min and max values for plot axes.
There was a problem hiding this comment.
Can you explain to me why it messed up setting min and max? I looked at the algorithm used and did not see it messing up the setting. Maybe I did not understand the algorithm.
There was a problem hiding this comment.
Your approach "if the zoom is in the old options, copy it to the new options" is correct. Just do it in XYPlotOptions and only use "zoom" field. Currently, every time we press "Apply" button in options dialog, zoom is gone. We want to preserve it, unless X or Y columns changed.
We use "zoom" field to track zoom and "boundaries" field to track data or user boundaries. "boundaries" is a calculated field and should never be set directly.
Because you clear boundaries, not zoom, the unzoom icon does not disappear when the chart is unzoomed.
When I said that using boundaries messes up setting min and max values, I was wrong. I noticed that zooming up and then setting min/max value does not work. But the real cause of this behavior is a bug in lines 376-377 of XYPlot. If both zoom and data boundaries are present, min/max of the two should be taken as an axis min/max. I would appreciate if you fix it too. (Or I can fix it later in your branch.)
src/firefly/js/charts/ChartsCntlr.js
Outdated
|
|
||
| if (!newOptions.zoom && oldOptions.zoom && !areAllValuesEaual(oldOptions.zoom,oldOptions.boundaries)){ | ||
| newOptions.zoom = oldOptions.zoom; | ||
| } |
There was a problem hiding this comment.
There should be no references to scatter plot specific options (zoom or boundaries) in Charts controller, which is designed to deal with all chart types.
lznakano
force-pushed
the
DM-9343-ChartZoom
branch
from
181bab4 to
8ac0c39
Compare
| @@ -107,7 +107,8 @@ export function resultsSuccess(callback, flds, tblId) { | |||
| shading: flds.shading || undefined, | |||
| x : { columnOrExpr : xName, error: xErr, label : xLabel, unit : xUnit, options : xOptions}, | |||
| y : { columnOrExpr : yName, error: yErr, label : yLabel, unit : yUnit, options : yOptions}, | |||
| tblId | |||
| tblId, | |||
| zoom, | |||
There was a problem hiding this comment.
Please, do not preserve zoom if X or Y column changed.
There was a problem hiding this comment.
You are right. When the columns change, the zoom should not be preserved. Thanks for pointing it out. I did not realize it.
Fixed bug after zoom
Pease check:
Thanks!