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.

Sign up for GitHub

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

Jump to bottom

[charts] Add a ChartsGrid component #11034

Merged
merged 14 commits into from
Feb 15, 2024
Merged

[charts] Add a ChartsGrid component #11034

merged 14 commits into from
Feb 15, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Nov 14, 2023 •
edited
Loading

Fix #10158

For documentation, I went with a page per chart linking to a longer explanation in the axis page.

Demos

Changelog

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Nov 14, 2023
@mui-bot
Copy link

mui-bot commented Nov 14, 2023 •
edited
Loading

Deploy preview: https://deploy-preview-11034--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 8da8ea1

@alexfauquette alexfauquette marked this pull request as ready for review January 26, 2024 14:46
@alexfauquette alexfauquette changed the title [charts][WIP] Experiment with grid component [charts] Experiment with grid component Jan 26, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette alexfauquette changed the title [charts] Experiment with grid component [charts] Add a grid component Feb 5, 2024
@alexfauquette alexfauquette mentioned this pull request Feb 5, 2024
Closed
alexfauquette
alexfauquette commented Feb 6, 2024
View reviewed changes
packages/x-charts/src/ChartsGrid/ChartsGrid.tsx
Comment on lines +77 to +78
const xTicks = useTicks({ scale: xScale, tickNumber: xTickNumber, tickInterval: xTickInterval });
const yTicks = useTicks({ scale: yScale, tickNumber: yTickNumber, tickInterval: yTickInterval });
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm relying on the fact that the grid use the same parametters and the same hook as axies ticks to get the alignement.

I though about doing the computation once and let axes and grid rely on it. But the benefit is not clear whereas the effort is important. So I decided to go that way, assuming it will be enough for at least a year, and if needed we will be able to review this process in the next major, the time to accumulate feedback

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 13, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

LukasTy
LukasTy reviewed Feb 14, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Very welcome and nice addition to the Charts customization! 💯
Great job. 👏

Leaving some API questions/considerations.
In addition to that, a couple of extra points:

  1. Have you considered the option/need to have conditional grid color styling?
    Wouldn't it be problem adding it in given the current API?
  2. Should we add the support for themeAugmentation? 🤔

docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/bars/bars.md Outdated Show resolved Hide resolved
docs/data/charts/bars/bars.md Outdated Show resolved Hide resolved
docs/data/charts/composition/composition.md Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsGrid/ChartsGrid.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsGrid/gridClasses.ts Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsGrid/ChartsGrid.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsGrid/gridClasses.ts Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsGrid/ChartsGrid.tsx Show resolved Hide resolved
@alexfauquette alexfauquette mentioned this pull request Feb 14, 2024
Closed
@alexfauquette
Copy link
Member Author

About extra points

Have you considered the option/need to have conditional grid color styling?
Wouldn't it be problem adding it in given the current API?

I don't see issues with the current API. The easies might be to put this color in the axis config, and add option to say if it applies to the tick label, the tick, the grid line

Should we add the support for themeAugmentation? 🤔

I always forget this aspect 🙃

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2024
LukasTy
LukasTy reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice work! 👏
IMHO, we can/should keep the themeAugmentation for a separate PR.
It's somewhat of a different topic than this PR, especially for other entries. 🙈
WDYT?

packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts Outdated Show resolved Hide resolved
@LukasTy LukasTy added the new feature New feature or request label Feb 15, 2024
LukasTy
LukasTy approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

🚀 💯

@LukasTy LukasTy changed the title [charts] Add a grid component [charts] Add a ChartsGrid component Feb 15, 2024
@alexfauquette alexfauquette enabled auto-merge (squash) February 15, 2024 10:27
@alexfauquette alexfauquette merged commit eba5169 into mui:next Feb 15, 2024
16 checks passed
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
@alexfauquette @LukasTy @thomasmoon
[charts] Add a ChartsGrid component (mui#11034)
0fb1b0d 
Signed-off-by: Alexandre Fauquette <[email protected]>
Co-authored-by: Lukas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasTy LukasTy LukasTy approved these changes

@flaviendelangle flaviendelangle Awaiting requested review from flaviendelangle

Assignees
No one assigned
Labels
component: charts This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[charts] Add grid layout
3 participants
@alexfauquette @mui-bot @LukasTy