Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix navigator #251

Closed
wants to merge 4 commits into from
Closed

Fix navigator #251

wants to merge 4 commits into from

Conversation

anajavi
Copy link
Collaborator

@anajavi anajavi commented Nov 21, 2019

This fixes navigator crash when updated (#228 (comment)).

The root cause was that navigator re-creates it's axes when navigator is updated.

There were several problems:

  1. 3e65a8d creates axis earlier, so that navigator can access the parent series axis
  2. b8e74fd allows provided axis to change, so that plotbands can react to axis changes
  3. cf538e7 forces plotbands to re-render when axis was changed
  4. e1d3ff1 fixes the navigator itself

Related issue on highcharts repository: highcharts/highcharts#12406

Probably more axis consumers, like Annotations and maybe Series need updating, but maybe we should test this in another alpha first?

@anajavi anajavi added this to the Version 4 milestone Nov 21, 2019
@anajavi anajavi requested a review from whawker November 21, 2019 15:11
@codecov-io
Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #251 into master will increase coverage by 6.65%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   87.81%   94.47%   +6.65%     
==========================================
  Files          82       68      -14     
  Lines        1026      832     -194     
  Branches      194      165      -29     
==========================================
- Hits          901      786     -115     
+ Misses        106       40      -66     
+ Partials       19        6      -13
Impacted Files Coverage Δ
...omponents/PlotBandLine/UsePlotBandLineLifecycle.js 100% <100%> (ø) ⬆️
...s/react-jsx-highcharts/src/components/Axis/Axis.js 88.88% <100%> (-1.12%) ⬇️
...act-jsx-highcharts/src/components/UseAxis/index.js 95.23% <92.3%> (-4.77%) ⬇️
...ts/src/components/BaseChart/createProvidedChart.js 42.85% <0%> (-42.86%) ⬇️
...ct-jsx-highstock/src/components/Navigator/index.js
packages/react-jsx-highstock/src/index.js
...rc/components/RangeSelector/RangeSelectorButton.js
...hstock/src/components/Navigator/NavigatorSeries.js
packages/react-jsx-highstock/test/test-utils.js
...sx-highstock/src/components/RangeSelector/index.js
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c60fa31...6eef1d0. Read the comment docs.

@whawker
Copy link
Owner

whawker commented Nov 21, 2019

If we rebase this branch on master, I'll publish an alpha from it, then after testing we decide whether to merge or not?

@anajavi
Copy link
Collaborator Author

anajavi commented Nov 21, 2019

If we rebase this branch on master, I'll publish an alpha from it, then after testing we decide whether to merge or not?

I rebased this. Any chance we can include #250 in the alpha too or just merge it to master first.

@whawker
Copy link
Owner

whawker commented Nov 21, 2019

Just this second merged #250, any chance you could rebase again? (Sorry!)

I'll publish one alpha from master, and one alpha from this branch.

@anajavi
Copy link
Collaborator Author

anajavi commented Nov 21, 2019

Just this second merged #250, any chance you could rebase again? (Sorry!)

I'll publish one alpha from master, and one alpha from this branch.

Rebased. And great!

@whawker
Copy link
Owner

whawker commented Nov 21, 2019

Published master as

  • react-jsx-highcharts@4.0.0-alpha.8
  • react-jsx-highstock@4.0.0-alpha.8
  • react-jsx-highmaps@2.0.0-alpha.8

And this branch as (note dev)

  • react-jsx-highcharts@4.0.0-dev.1
  • react-jsx-highstock@4.0.0-dev.1
  • react-jsx-highmaps@2.0.0-dev.1

@anajavi
Copy link
Collaborator Author

anajavi commented Nov 21, 2019

react-jsx-highcharts@4.0.0-dev.1 crashes LineSeries, but only sometimes. I'll try to make a reproduction of it.

@anajavi
Copy link
Collaborator Author

anajavi commented Dec 3, 2019

This won't work.

Under some conditions react runs the render twice on mount. On second render the axisRef.current does not keep the previous renders value. This leads to creating the axis on both renders.

So this PR needs to be scrapped. Also the ColorAxis needs to be fixed to use useEffect too.

I am out of ideas how to fix the Navigator now.

@anajavi anajavi closed this Dec 3, 2019
@anajavi anajavi deleted the axis-rework branch January 17, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants