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

Can't put a chart in an external component #417

Closed
1 of 3 tasks
mattiaz9 opened this issue Aug 4, 2020 · 8 comments
Closed
1 of 3 tasks

Can't put a chart in an external component #417

mattiaz9 opened this issue Aug 4, 2020 · 8 comments
Assignees
Labels
pinned question Further information is requested

Comments

@mattiaz9
Copy link

mattiaz9 commented Aug 4, 2020

I'm submitting a...

  • bug
  • feature
  • chore

What is the current behavior

When I try to put a Chart in an external component I get this error:
TypeError: Cannot destructure property 'origin' of 'this.context.chartConfig.find(...)' as it is undefined

Looks like a Chart must be a direct descendant of the ChartCanvas component.

What is the expected behavior

We should be able to organize the code using external components as it could get pretty messy.

Please tell us about your environment

  • Version: 1.0.0-alpha.7
  • Browser: [all | Chrome XX | Firefox XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari ]

Other information

Could this be related to the old context api?

@markmcdowell
Copy link
Collaborator

Hi there, hopefully I'm understanding you correct. The ChartCanvas is the 1 component this is always required; it's creating the canvas dom elements for everything to draw on, it's also creating the context that other components rely on.

You should be able to just wrap the Chart in ChartCanvas. The smallest possible code would be:

<ChartCanvas
    height={height}
    ratio={ratio}
    width={width}
    data={data}
    displayXAccessor={displayXAccessor}
    seriesName="Data"
    xScale={xScale}
>
    <Chart />
</ChartCanvas>

@markmcdowell markmcdowell self-assigned this Aug 4, 2020
@markmcdowell markmcdowell added the question Further information is requested label Aug 4, 2020
@mattiaz9
Copy link
Author

mattiaz9 commented Aug 4, 2020

Correct. But it cannot be like this:

<ChartCanvas
    height={height}
    ratio={ratio}
    width={width}
    data={data}
    displayXAccessor={displayXAccessor}
    seriesName="Data"
    xScale={xScale}
>
    <MyCustomChart id={1} />
</ChartCanvas>

Where MyCustomChart is something like:

const MyCustomChart = ({ id }) => {
  return (
    <Chart id={id}>
      ...
    </Chart> 
  )
}

@markmcdowell
Copy link
Collaborator

Aha yes I seem what you mean, it's definitely related to the legacy context api (which I need to remove). My first guess would be if it was a class component it may work. I'm not too familiar with how the legacy one works, hard to find docs on it.

@mattiaz9
Copy link
Author

mattiaz9 commented Aug 4, 2020

I tried with a class component but I got the same error.

How long will it take to migrate to the new context api? is it something we can expect in the next pre-release?

@markmcdowell
Copy link
Collaborator

The intention is that v1 will be using the new context api, which is in the next couple of weeks.

@stale
Copy link

stale bot commented Aug 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The issue has not had any activity for 7 days label Aug 11, 2020
@stale stale bot removed the stale The issue has not had any activity for 7 days label Aug 11, 2020
@abforce
Copy link

abforce commented Aug 24, 2020

I also faced this issue. This is because of the ChartCanvas iterating its children using React.Children.map and it expects all its children to be of type Chart. Here's the code:

https://github.com/reactivemarkets/react-financial-charts/blob/c3985d5ee96fcbd5ad5a922df595d31930d0cee5/packages/core/src/utils/ChartDataUtil.ts#L54

@mattiaz9 As a workaround you can organize your custom component to return a React.Fragment not a Chart. This way you can add a Chart to a ChartCanvas and use your custom component as its only child:

function CustomChart(){
    return (
        <>
             <XAxis/>
             {/* other components */}
        </>
    );
}

Usage:

<ChartCanvas>
    <Chart><CustomChart/></Chart>
</ChartCanvas>

markmcdowell added a commit to markmcdowell/react-financial-charts that referenced this issue Aug 27, 2020
Checking props for id to allow external components as mentioned in react-financial#417.
markmcdowell added a commit that referenced this issue Aug 27, 2020
Checking props for id to allow external components as mentioned in #417.
@markmcdowell
Copy link
Collaborator

Closing as this is fixed in the latest alpha versions, the custom component needs to have at least an id prop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned question Further information is requested
Development

No branches or pull requests

3 participants