DM-7920 Refactor chart data and generalize actions #206
Conversation
| const chartActions = [CHART_UI_EXPANDED,CHART_MOUNTED,CHART_UNMOUNTED, | ||
| CHART_ADD,CHART_REMOVE,CHART_DATA_FETCH,CHART_DATA_LOADED, CHART_OPTIONS_UPDATE, | ||
| TablesCntlr.TABLE_REMOVE, TablesCntlr.TABLE_SELECT]; | ||
|
|
There was a problem hiding this comment.
I think this forces adding watched actions in 2 places without much gain.
So, instead of running through the switch statement once, it does an indexOf... and if it matches, it'll still run through the switch statement.
| * @public | ||
| * @function dispatchZoom | ||
| * @memberof firefly.action | ||
| */ | ||
| export function dispatchZoom(chartId, tblId, selection) { | ||
| const {xyPlotData, xyPlotParams, decimatedUnzoomed} = get(getChartSpace(SCATTER), chartId, {}); | ||
| export function setZoom(chartId, chartDataElementId, selection=undefined) { |
There was a problem hiding this comment.
Is this on purpose where you change function name to setZoom but, not the @function name?
There was a problem hiding this comment.
Also, I find that it's not a good idea to have too much logic in a dispatch function. It should be in the action creator, reducer, or a saga. This way, the action can be dispatched from a function like this or dispatched by an event listener action on an action event. ie. remote api.
- moved adding a default chart to FireflyViewerManager - renamed XYPlotCntlr to ChartDataTypeXYCols - renamed HistogramCntlr to ChartDataTypeHistogram - a bit of cleanup in docs
| * @function dispatchChartAdd | ||
| * @memberof firefly.action | ||
| */ | ||
| export function dispatchChartAdd({chartId, chartType, chartDataElements, groupId='main', deletable, help_id, dispatcher= flux.process}) { |
There was a problem hiding this comment.
You should default as much as possible. In this case deletable=true and help_id=something
| getUpdatedOptions = ( cdt.getUpdatedOptions || ((opts) => {return opts;}) ), | ||
| fetchData=cdt.fetchData, | ||
| fetchParamsChanged = ( cdt.fetchParamsChanged || (() => {return !isUndefined(fetchData);}) ), | ||
| } = chartDataElement; |
There was a problem hiding this comment.
This is some really complicated code. Maybe better todo:
const getUpdatedOptions = chartDataElement.getUpdatedOptions || cdt.getUpdatedOptions || (opts) => opts;
const fetchData= chartDataElement.fetchData || cdt.fetchData;
const fetchParamsChanged = chartDataElement.fetchParamsChanged || cdt.fetchParamsChanged || () => !isUndefined(fetchData) )
| options: xyPlotParams, | ||
| tblId | ||
| } | ||
| ], dispatcher}); |
There was a problem hiding this comment.
I am concerned about this as a utility routine. I am not sure we want to make it an example. It is easier to understand the code if dispatches are being used directly. loadXYPlot is effectively a dispatch but you can't tell by looking at the code.
a couple of other things:
'scatter'should type a a constant.- why not just say
type: DATATYPE_XYCOLS.id?
Being picky because all this code will be examples for all the other charts.
| serverParams.push(histogramParams.falsePositiveRate); | ||
| //serverParams.push(histogramParams.minCutoff); | ||
| //serverParams.push(histogramParams.maxCutoff); | ||
| return serverParams; |
There was a problem hiding this comment.
why not?
if (!histogramParams) { return []; }
return [
histogramParams.columnOrExpr,
histogramParams.x && histogramParams.x.includes('log'),
histogramParams.numBins,
histogramParams.falsePositiveRate,
];
| */ | ||
|
|
||
| const chartDataTypes = []; | ||
|
|
There was a problem hiding this comment.
I am struggle with this code. I am really torn. It is effectively keeping application state outside of the store. I solved this problem with drawing layer by defining them in ReduxFlux.js (line 110) and them creating the reducing function with the factory (line 129). However, it does make things more complex. Might be worth Tatiana, Loi, and Trey talking about tomorrow.
…m to action creators - move code defining chart data types under dataTypes directory - support optional fetch on table sort - other minor cleanup in response to review comments
Refactored chart actions and data to be able to support multiple chart elements connected to different data.
charts.data - is the store object with chart ids as the keys, the values are chart data object, which has chartDataElements field.
chartDataElements - object with chartDataElementId keys
and chartDataElement object as value
chartDataElement shape:
{id, options, defaultOptions, data, meta, tblId,
type - to get helper functions like
chartDataElements - object with chartDataElementId keys
and chartDataElement object as value
chartDataElement shape:
{id, options, defaultOptions, data, meta, tblId, type}
The 'type' is an id of a ChartDataType, two of them are predefined: 'xycols' and 'histogram'.
Each chart data type has fetchData function and fetchParamsChanged functions, and possibly other helper functions.