React JSX Highcharts v4 Alpha Feedback (Hooks rewrite) #228
Comments
|
I had an issue trying to updating chart series while rendering a PlotLine:
I'll try to make an stackblitz of it as soon as I can but right now I'm a little bit busy, I hope this can be useful. |
|
@leonardolessa don't bother with the stackblitz. I already found the bug. It should be axis.removePlotBandOrLine not removePlotLine. Thank you very much for trying out the alpha! edit: fixed and added test in 41c028d |
|
@whawker Can you make a new alpha release when you have time? There's a couple of fixes and optimizations |
|
Yep of course. FYI The funeral was yesterday, so I should be more available to assist from now on. |
|
Sorry to hear about your loss. Take your time. |
|
Thanks @anajavi Published:
All using NPM tag |
|
Just looking into an issue with I think it's something to do with the |
|
RangeSelectorInner never gets axis from useAxis because it's not inside Axis or provide axisId for useAxis. That's why it does not render. |
|
Oh wow, nice one, thanks! |
|
I've run in to an issue with a highcharts plugin for draggable plot lines. Working in Broken in The line is holding on to the initial value in |
The Plotline seems to have broken update lifecycle. I will fix this. |
|
@jhartling PlotLine lifecycle fixed in 3b318de |
|
@jhartling I modified your Highcharts plugin to return plotline created. It now works with alpha.4 |
|
if we export usePlotline, the draggable plotline could be implemented cleanly in react component without patching Highcharts. non-working example: const MyDraggablePlotLine = props => {
const plotline = usePlotLine();
draggablePlotLine(plotline);
}usage: <PlotLine value={220} width={10}>
<MyDraggablePlotLine />
</PlotLine> |
|
@anajavi Awesome, thanks for your work on this great library! |
|
@whawker how do you feel about prop-types? I don't know if they add any value now that the HOCs are removed. |
|
I'm in two minds, largely anything you can do with the API outlined at https://api.highcharts.com you can do with our library, so maybe we can just refer people there. We do have the noticeable differences though, in regard to "text as children" and events as Maybe we could drop prop types and add more notices to the developer, like we do for missing Highcharts modules? For instance, if we see an |
|
Removing the prop types is not terribly important, and I don't think it is breaking change. So it doesn't need to be decided now. |
|
Just noting a few more issues the alphas, so they don't get missed.
And obviously, there is a ton of documentation in the wiki and Readmes to update. |
This is side-effect of replacing isEqual with Object.is. The example works by modifying the example to: const dataLabels = {
format: '<div style="text-align:center"><span style="font-size:25px;color:black">{y}</span><br/><span style="font-size:12px;color:silver">km/h</span></div>',
y: -50
};
const tooltip = {
valueSuffix: ' km/h'
}
<SolidGaugeSeries
name='Speed'
data={[ this.state.kmph ]}
dataLabels={dataLabels}
tooltip={tooltip}
/>Without constant objects in the props the Series-component keeps calling series.update, which seems to cause re-animating the gauge.
We could make an example of the draggableplotline being mounted with usePlotBandLine? |
Fixed in 9e3ccd2 |
|
Now SplineWithPlotBands example does not work when using minified production build. It works with non-minified development mode build. I suspect there is something wrong with the minifier. |
|
SplineWithPlotBands fixed in abcdbbe |
|
Look like I'm going to have to revert this commit as they are used by the other packages to test the components. https://github.com/whawker/react-jsx-highcharts/blob/master/packages/react-jsx-highstock/test/components/RangeSelector/RangeSelector.spec.js#L23-L29 |
I can rewrite those tests without the contexts. It can be done by mocking the hooks instead. Sorry for the trouble, I forgot to check the tests. |
Tests were rewritten to mock hooks in 9d96859 |
|
About the docs. How about trying out docusaurus. It can pull markdown docs from github and apparently it can also deploy to gh-pages. Used by quite many projects. It might even be able to embed stackblitz examples, but I haven't tried. |
|
I've rewritten the Custom component example using our exposed hooks - it works really nicely! Much easier than before.
Interesting, might need to give this one some thought, I fear we might get quite a few issues raised for these types of things. Obviously avoiding the creation of objects in render is best practice, but I don't believe many people actually follow this particular practice.
docusaurus look good, just having a read of the how-tos as we speak. |
I agree. I also really like the logic sharing that hooks give. Do the examples live in that repository nowadays? What about the examples in this repository? note: You might want to add useCallback to handleFromDateChange to prevent re-renders of DayPicker.
This is true. With Object.is it is however possible to optimize those re-renders. With very large datasets the re-renders caused lots of expensive calls to isEqual (same data is the most expensive to check).
The react-leaflet is same kind of wrapper for Leaflet as this project is for Highcharts. It uses docusaurus, so maybe same kind of format would work there? Especially linking from component docs to Highcharts docs about the options. |
Isn't that example actually the same as the whole project of react-jsx-highstock-datepickers? Nicely done indeed! |
The examples in each are basically the same - the ones in this repo are static generated, so we can display them on https://whawker.github.io/react-jsx-highcharts/examples - whereas the other repo is a create-react-app so I can test everything works properly in a real life use case.
Good idea, big fan of that project (although I have never had cause to use it) - I liked how they attacked their problem in a similar way to ours - until the hooks rewrite anyway!
Basically yes, although that project allowed for a bit more customisation. |
|
@anajavi just pushed a Also includes a potential new logo? |
|
Whoaa! A Really, really nice site you made! I quickly tried to embed stackblitz with an iframe in the site, and it kind of works too. Really impressed, nice work @whawker ! |
|
I'm hitting some issues in the alpha with the Navigator component. Resizing the navigator produces weird behaviour like it's changing the axis type / date range before ultimately freezing. It's also throwing an error when updating Navigator props. |
|
@jhartling can you please paste the error here? |
|
|
@whawker what do the following lines in navigator do? I think the cloning might somehow make the navigator children to refer to wrong axis, but I am not sure. |
|
Navigator problem turned out to be a little more complex than simply cloning elements. Updating navigator destroys it first, and then runs init: Init then recreates the navigator axis: After the navigator axis is recreated, the This can be fixed by always getting the newest axis object from Highcharts before updating it: const NavigatorAxis = ({ children, axisId, ...restProps }) => {
const chart = useChart();
const renderedRef = useRef(false);
useEffect(() => {
const axis = chart.get(axisId);
if (!axis) return;
updateNavigatorAxis(getNonEventHandlerProps(restProps), axis);
}, [chart]);Also NavigatorAxis should create AxisContext to pass axis to its children. |
|
While trying to fix Navigator component, I found a couple of other issues in Highstock navigator: highcharts/highcharts#12406 |
|
Apologies for the lack of input here, life has been massively hectic the past few weeks. Thanks for looking into the navigator issues. It looks as though cloneElement can be deleted, it's left over from some poor refactoring from v2 to V3 when we used to pass down |
|
I've tried to fix the navigator in various ways. But it seems that getting navigators xAxis to work consistently is hairy. The navigator provides usable xAxis when navigator is first created. Any update to navigator causes the navigator axis to be recreated. After recreation trying to get the new axis instance from Highcharts is not consistent. Might be timing related problem. |
|
I think I found a way to make navigator work. The fix is going to touch almost all parts of the code that deals with axes. Also I think axis and series updates needs to be changed so that they are executed on component render instead of in useEffect. I am not entirely sure that the navigator can be fixed for all use cases, but let's see.. |
|
v4.0.0-alpha.8 has the following new features:
<ColorAxis minColor="#FAA" maxColor="#AAF">
<Series data={..} />
</ColorAxis>
let randomString=Math.random().toString(36)
<Chart onClick={(event) => {console.log(randomString)}} />The above is bad practice though, but sometimes needed. @jhartling the navigator fix is in v4.0.0-dev.1. It's still has some rough edges and one hard to trigger bug. If you like to test it, be sure to check that both react-jsx-highcharts and react-jsx-highstock are the same version by doing: $ npm ls react-jsx-highcharts ; npm ls react-jsx-highstock
├── react-jsx-highcharts@4.0.0-dev.1
└─┬ react-jsx-highstock@4.0.0-dev.1
└── react-jsx-highcharts@4.0.0-dev.1
|
|
The v4.0.0-dev.1 ended up being too broken. Strangely enough I can't anymore reproduce the Navigator crash with v4.0.0-alpha.8. |
|
Published
Although I forgot to update the Readme's doing that now |
|
I added release notes draft. Maybe we should close this issue and open a new feedback issue for the final release? |
|
This issue is superseded by #259 |
|
Clearly we think very alike! @anajavi I left you a note on your release notes draft. |
This issue exists to feedback any issues discovered while testing the new version 4 alphas.
Please note, this rewrite is based on React Hooks, so requires React 16.8 as minimum. Please see the release notes for more information on breaking changes.
The text was updated successfully, but these errors were encountered: