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

[charts] Accept component in labelMarkType #16739

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Feb 26, 2025

Related to #16640. Implementation of option B as defined here.

Accept component in labelMarkType to customize the mark in legends and tooltips.

A consequence of this API is that when customizing the shape of a scatter chart, a user will need to define the labelMarkType and scatter shape in different places.

Screen.Recording.2025-02-26.at.10.53.58.mov

Comment on lines +9 to +13
export interface ChartsLabelCustomMarkProps {
className?: string;
/** Color of the series this mark refers to. */
color?: string;
}
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 wonder if we should pass the seriesId here. Otherwise if someone is using the same component for all series they won't know which series is rendering the current component.

Copy link
Member

Choose a reason for hiding this comment

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

Could be an option yeah

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexfauquette what's your take?

If the decision is affirmative, I'll do that in a follow-up PR so I can merge this PR faster.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason to modify the component based on the id. The series type already make more sense.

I fear that with passing ids, people start do do complex stuff relying on ids, and then realise small id change can break their entire legend design

Comment on lines +43 to +44
width: 14,
height: 14,
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to define a default size here so that the custom component defaults to occupying 100% of the width/height.

Copy link
Member

Choose a reason for hiding this comment

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

You would need to set the height on labelMarkClasses.line as unset. It added a few pixels to the demos, hence all the changes 🫠

@mui-bot
Copy link

mui-bot commented Feb 26, 2025

Co-authored-by: Jose C Quintas Jr <[email protected]>
Signed-off-by: Bernardo Belchior <[email protected]>
@bernardobelchior bernardobelchior marked this pull request as ready for review February 26, 2025 14:20
@bernardobelchior bernardobelchior merged commit 1613993 into mui:master Feb 27, 2025
18 checks passed
@bernardobelchior bernardobelchior deleted the legend-custom-mark-type branch February 27, 2025 07:37
@zannager zannager added the component: charts This is the name of the generic UI component, not the React module! label Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants