search

esnet / react-timeseries-charts

Declarative and modular timeseries charting components for React
http://software.es.net/react-timeseries-charts
Other
861 stars 284 forks source link

TimeAxis and YAxis do not rerender after style prop changes #370

Closed mikol closed 5 years ago

mikol commented 5 years ago

šŸ›Bug report

Describe the bug Both TimeAxis and YAxis aggressively filter updates. They donā€™t rerender via React ever (shouldComponentUpdate() always returns false and render() is only used to insert an SVG g element). They include a set of gates in componentWillReceiveProps() to determine granularly which elements should be updated. These gates donā€™t account for all possible prop changes, which is why we donā€™t see the components rerendering in this and other cases.

See also: https://github.com/esnet/react-timeseries-charts/issues/346

To Reproduce Steps to reproduce the behavior:

  1. Set ChartContainer timeAxisStyle prop (e.g., {values: {fill: '#c00'}})
  2. Change ChartContainer timeAxisStyle prop on any subsequent render (e.g., {values: {fill: '#fc0'}})
  3. Note that the time axis styles like tick label fill do not update

Follow a similar procedure with YAxis style prop and note that the axis styles like label fill do not update.

Expected behavior TimeAxis and YAxis should rerender whenever they receive changed props.

Desktop (please complete the following information):

khoama commented 5 years ago

It looks like the YAxis is set to always ignore new prop changes. Not sure if this is a mistake or not https://github.com/esnet/react-timeseries-charts/blob/master/src/components/YAxis.js#L178

mikol commented 5 years ago

@khoama ā€“ It seems intentional to me. The initial render adds an SVG g element, and then updates are managed manually via componentWillReceiveProps().

pjm17971 commented 5 years ago

The axis code still uses d3 to render the chart. When all prop changes caused a re-render of the axis code this featured very high in profiling we did (this was years ago now though). So, we started being selective about the updates as well as some updates doing a more minimal DOM manipulation where possible. This made things a lot faster, at least for the majority of use cases.

That's the background here.

Ultimately we wrote react-axis so that we could replace this d3 code and not have to deal with this, but we're lacking the resources to finish that work right now. I am hopeful we can make some progress on this project in the coming 6 months though as there's some internal drivers for that.