Closed ivan-aksamentov closed 4 years ago
I am not that up with React but I had a little play and it seems to me that this component only needs to re-render the graph if the time series changes, which is defined by the data
property, or when the size of the graph changes, which is defined by the width
and height
properties. Yes?
So I changed https://github.com/neherlab/covid19_scenarios/blob/b07eeaedd82dd3f54067e241cb62e5e2ac879504/src/components/Main/Containment/ContainmentGraph.tsx#L229 to be:
useEffect(() => draw({ data, onDataChange, width, height, d3ref }), [data, width, height])
and that seems to solve the problem. Now the graph only re-renders if the start and end dates change, or the number of points changes (this triggers a recalculation of data
) or the window is resized (this updates width
and height
).
Also previous to this the graph is re-rendering on just about every event, not only focus events but all other kinds of events that are not related to this graph.
Happy to create a PR for this if it is agreed that this is the correct solution...
Oops! Of course now the graph is not rendered correctly at application start up... :-(
Ahah! So it seems that the first time draw()
is called we have d3ref.current
is null
, which causes an error and so the effect is not run again. Adding a guard against this and adding d3ref.current
as a dependency for the effect seems to solve the problem:
function draw({ data, width, height, onDataChange, d3ref }: DrawParams) {
if (d3ref.current === null) {
return
}
...
useEffect(() => draw({ data, onDataChange, width, height, d3ref }), [data, width, height, d3ref.current])
By the way, as I said I am not that hot on React development but I found that I needed to change the webpack.SourceMapDevToolPlugin
to set noSources
to false when debugging in order to get half-decent debugging experience. Is there a better way to manage this?
Regarding source maps: https://github.com/neherlab/covid19_scenarios/pull/94#issuecomment-602126275
Steps To Reproduce
The current behavior
The "Mitigation" plot dot being dragged is not moving until mouse button is up
The expected behavior
The "Mitigation" plot dot being dragged is not moving as the cursor moves
Hypotheses
The blur event out of the field coincides with the initial click on the plot's dot. After the blur, the form rerenders and also causes the plot to rerenders. Currently there is a hack. This
.html(null)
on every render clears the entire plot:https://github.com/neherlab/covid19_scenarios/blob/b07eeaedd82dd3f54067e241cb62e5e2ac879504/src/components/Main/Containment/ContainmentGraph.tsx#L44-L49
This also causes the destruction of the black dot we are dragging. The new dot will be rendered only on mouse up. Meanwhile we are dragging nothing.
Potential solution
We need to remove the hack shown above and to figure out proper dependencies for the `useEffect hook here:
https://github.com/neherlab/covid19_scenarios/blob/b07eeaedd82dd3f54067e241cb62e5e2ac879504/src/components/Main/Containment/ContainmentGraph.tsx#L225-L230
such that the plot is properly cleared when it is needed, that it is responsive (to screen size) and not causing unnecessary rerenders at the same time
D3-React interop in this component should be properly audited for additional bugs and performance problems.