DM-6286 Charts Container #216
Conversation
| } | ||
| } | ||
|
|
||
| function renderToolbar(chartId, expandable, expandedMode, toggleOptions) { |
There was a problem hiding this comment.
Just a thought...
These render* functions are basically react's functional components. Would it be better written as such? ie.
Toolbar({chartId, expandable, expandedMode, toggleOptions})
usage:
const {Toolbar, Options} = HISTOGRAM_TBLVIEW;
<Toolbar />
<Options />
There was a problem hiding this comment.
I did make Chart, Toolbar, and Options functional components first, but then I wanted to switch to non-functional component for one chart type, and realized I can not pass it as a function anymore. May be it's not a big deal, and the recipe would be wrapping it into a functional component.
| <div className='PanelToolbar_group'> | ||
| {renderSelectionButtons(chartId, tblId, tableModel)} | ||
| </div> | ||
| <div className='PanelToolbar_group'> |
There was a problem hiding this comment.
I think you mean to use 'PanelToolbar__group'(double underscores).
|
|
||
| import * as ChartsCntlr from '../ChartsCntlr.js'; | ||
|
|
||
| class ChartPanelView extends Component { |
There was a problem hiding this comment.
Consider exposing ChartPanelView if it can be used separately without ChartPanel.
If so, it should be in its own file.
There was a problem hiding this comment.
Is there a use case for ChartPanelView alone? It my help be to know so I can see the reuse better.
There was a problem hiding this comment.
The only use case I can think of is a chart, which has no dependency on the redux store and is not part of any multi-chart viewer. In this case, I think I should move dispatchChartMount/Unmount from ChartPanelView component to ChartPanel.
| ); | ||
| })} | ||
| </div> | ||
| const {chartId, expandedMode, closeable} = this.props; |
There was a problem hiding this comment.
I don't quite get the purpose of this component. It's a simple wrapper around ChartPanel and MultiChartViewer without any added logic of its own. When do we use this?
There was a problem hiding this comment.
This component handles single chart display from API (both standard and expanded mode) and wraps the default MultiChartViewer. Perhaps, at some point it can be eliminated, but I like to have this wrapper so that only chart code changes when chart code is refactored.
| renderToolbar, | ||
| getChartProperties, | ||
| updateOnStoreChange | ||
| }; |
There was a problem hiding this comment.
Is there a typedef for what goes into this object?
|
|
||
| import * as ChartsCntlr from '../ChartsCntlr.js'; | ||
|
|
||
| class ChartPanelView extends Component { |
There was a problem hiding this comment.
Is there a use case for ChartPanelView alone? It my help be to know so I can see the reuse better.
| <PagingControl activePlotId={activePlotId} | ||
| visRoot={visRoot} | ||
| expandedMode={expandedMode} /> | ||
| <PagingControl |
There was a problem hiding this comment.
You might want to break PagingControl into is own file.
- Moved 'Close' button to the extreme left - Chart remove icon is now on the chart - Use Chart, Options, Toolbar functional components instead of renderChart, renderOptions, renderToolbar
The major classes - MultiChartViewer, MultiChartToolbar
Also refactored charts display. ChartPanel.jsx is now generic and gets info via chartType.
See chartTypes/HistogramTblView and chartTypes/ScatterTblView