Closed jameshiebert closed 6 years ago
I put in a similar but more detailed comment in review, as follows:
React strongly favours declarative coding. This code does a lot of things procedurally where it could, and should, do them declaratively. As part of that it also misses opportunities to extract components. Specifically:
Variables containing content to be rendered are built up procedurally, and then used in the return from render()
. (More specifically, tabbed container contents are built up like this.) React documentation occasionally shows such code, but it is for simple demonstration cases.
Several parts of the built-up content should be extracted as components. Key instances of this are the core content built up in variables annualTabPanel
, longTermTabPanel
, timeseriesTabPanel
, contextTabPanel
. The tab panel contents should in each case be encapsulated in a component, e.g., AnnualCycleGraph
, LongTermAverageGraph
, etc. Each such component will be passed props containing the data and callbacks it uses.
I see that part of what's going on here is that, depending on conditions, a tab variables are either assigned or left unassigned, and only the assigned ones get rendered. For simple cases, this is OK, but it's hard to read when there is a lot of code between variable assignment and render as there is now. (After you extract various components, it may be short enough to become readable again.) A more declarative version of this pattern is the inline if with logical && operator::
render() {
return (
<Parent>
{
condition &&
<Component prop=...>... </Component>
}
...
</Parent>
);
}
Component
and its children are rendered if and only iff condition
is truthy.
Do this after we have replaced react-bootstrap-tabs with React Bootstrap Tabs. Will not have to separate the creation of tab labels and tab panels. They are unified, and that will make writing the conditional code considerably simpler.
Starting this now and will rebase onto master after https://github.com/pacificclimate/climate-explorer-frontend/pull/117 is merged.
From the conversation in PR #117:
Agreed. We need to split the plots into each being their own component, accepting only what data they need and rendering as required.