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

Chart component #2081

Merged
merged 28 commits into from
Jul 14, 2023
Merged

Chart component #2081

merged 28 commits into from
Jul 14, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented May 26, 2023

New chart component. Allows for composing line chart, bar chart, area chart and scatter chart, with independent data definitions for each chart.
Uses recharts, can gradually be moved to the new MUI X charts library, let's create an issue for that once this is merged.

Closes #789
Related discussion: #2005

Screen.Recording.2023-07-12.at.23.31.21.mov

@apedroferreira apedroferreira added the feature: Components Button, input, table, etc. label May 26, 2023
@apedroferreira apedroferreira self-assigned this May 26, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 26, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 2, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 6, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 7, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2023
@apedroferreira apedroferreira changed the title (WIP) Chart component Chart component Jul 12, 2023
@apedroferreira apedroferreira marked this pull request as ready for review July 12, 2023 22:38
import { usePageEditorState } from '../AppEditor/PageEditor/PageEditorProvider';
import { useDom, useDomApi } from '../AppState';
// eslint-disable-next-line import/no-cycle
import BindableEditor from '../AppEditor/PageEditor/BindableEditor';
Copy link
Member

@Janpot Janpot Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we can solve all these cycles by creating a react context very high up in the toolpad app that holds a registry of all available propertyControls. It would still be a cyclic dependency, but one that we explicitly enabled through react API.

I'm ok with a sporadic // eslint-disable-next-line import/no-cycle, but it's clear that this is going to become very infectious. Feels a bit like a code smell to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's becoming a bigger issue when we add controls with bindable props, I was focused on functionality first but will see if I can find a way to fix this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's too much we can do it in follow up, but I believe a context should be able to solve this in a quite straightforward way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the cyclical dependency with prop controls was simpler than the other one with the binding editors - so passing the prop controls as context does fix the issue, nice one! I had thought it was part of the same problem.

I put the list of prop controls to pass to the context in the ComponentEditor component - let me know if you think this should be done differently in terms of file structure.

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good

@apedroferreira apedroferreira merged commit e617faf into mui:master Jul 14, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Components Button, input, table, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Chart component
2 participants