-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[charts] Add charts toolbar with zoom options #17615
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
[charts] Add charts toolbar with zoom options #17615
Conversation
Thanks for adding a type label to the PR! 👍 |
Localization writing tips ✍️Seems you are updating localization 🌍 files. Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️
Deploy preview: https://deploy-preview-17615--material-ui-x.netlify.app/ Updated pages: |
0ea597f
to
6e9cad8
Compare
CodSpeed Performance ReportMerging #17615 will not alter performanceComparing Summary
|
89d6ed8
to
5a4ec1e
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
5a4ec1e
to
c8e7f3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely. I've added some comments on things I didn't understand or where the API could be improved for discussion
packages/x-charts-pro/src/ChartsToolbarPro/internal/ChartsToolbarZoomInButton.tsx
Show resolved
Hide resolved
packages/x-charts-pro/src/internals/plugins/useChartProZoom/useChartProZoom.types.ts
Show resolved
Hide resolved
export interface ChartsIconSlots { | ||
/** | ||
* Icon displayed on the toolbar's zoom in button. | ||
* @default ChartsZoomInIcon | ||
*/ | ||
zoomInIcon: React.ComponentType<IconProps>; | ||
/** | ||
* Icon displayed on the toolbar's zoom out button. | ||
* @default ChartsZoomOutIcon | ||
*/ | ||
zoomOutIcon: React.ComponentType<IconProps>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in the pro package?
Or something like
export interface ChartsIconSlotsExtension {}
export interface ChartsIconSlots extends ChartsIconSlotsExtension { }
To allow doing
declare module '@mui/x-charts' {
interface ChartsIconSlotsExtension {
/**
* Icon displayed on the toolbar's zoom in button.
* @default ChartsZoomInIcon
*/
zoomInIcon: React.ComponentType<IconProps>;
/**
* Icon displayed on the toolbar's zoom out button.
* @default ChartsZoomOutIcon
*/
zoomOutIcon: React.ComponentType<IconProps>;
}
}
A bit similar to #14063
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it, but I'm now reconsidering. We are still exporting the pro icons from the community package, so there isn't a big benefit in extending the types, is there?
Another option would be for the pro package to export a ChartsSlotsPro
type which would extend from ChartsSlots
. That way we could move the pro-only components to that package instead of increasing the bundle size of the community version.
If you think that's a good idea, I could do it in a follow-up PR, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the pro slots to the pro package
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
c5d2d3b
to
f6bdb8c
Compare
f6bdb8c
to
5fe4a50
Compare
5fe4a50
to
2c5417b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick, but otherwise looking good
|
||
export type ChartsSlots = ChartsBaseSlots & ChartsIconSlots; | ||
|
||
export const materialSlots: ChartsSlots = { ...baseSlots, ...iconSlots }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit weird we export materialSlots
, as it is technically not a slot. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you say it isn't a slot?
At the moment, it might not be, be it'll be in a follow-up PR.
We'll use it like this:
<ScatterChartPro
{...params}
xAxis={[{ zoom: true }]}
yAxis={[{ zoom: true }]}
showToolbar
slots={{ baseIconButton: CustomIconButton }}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the slot is the part that can be overridden. So the slot is aptly named a "slot", where something else (a component in this case) fits into.
This means the items here are not slots, they are components that are used in the slots by default. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion for the name? I can change it in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is to have a single object import, I would go with materialComponents
maybe?
But my preferred way would be
export { IconButton } from '@mui/material/IconButton'
since the object way probably doesn't treeshake properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of defaultSlotsMaterial
?
But my preferred way would be
What do you mean? I thought you meant the constant. Didn't get what you meant 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultSlotsMaterial
can work.
What do you mean? I thought you meant the constant. Didn't get what you meant 😅
Yeah, IconButton
is a component. My secondary point with that comment is that we are exporting an object here which is a list of components, which probably breaks treeshaking, since whenever any of the components is used it will load all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll use that 👍
My secondary point with that comment is that we are exporting an object here which is a list of components, which probably breaks treeshaking, since whenever any of the components is used it will load all of them
It'll break tree-shaking nevertheless because we need to select the component in runtime. We'll never be able to remove the code from those components from the bundle because if a slot goes missing in runtime we'll need to use the default from MUI.
That's one of the conclusions I arrived at here, but decided to keep the same API as the Data Grid.
If we want to fix the tree-shaking issue we need to find another way to register the default slots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR: #17710
Add charts toolbar with zoom options. Part of #17557.
Designs.
Screen.Recording.2025-04-30.at.10.27.52.mov
This PR closely follows the Data Grid's toolbar implementation. This PR does not yet implement customization of the toolbar, which will be done in subsequent PRs, along with other improvements such as defining the toolbar's position.