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] Pass all props to legend #14392

Merged
merged 5 commits into from
Sep 2, 2024

Conversation

JCQuintas
Copy link
Member

As part of #14231 I've noticed some props weren't properly documented nor passed to the components.

Allowing these props should make small adjustments simpler for the user.

Also made prop inheritance a bit simpler

interface ChartsLegendProps extends LegendRedererProps {}

interface LegendRedererProps extends LegendPerItemProps {}

As opposed to previously where

interface ChartsLegendProps {}

interface LegendRendererProps extends LegendPerItemProps {}

interface LegendPerItemProps extends ChartsLegendProps {}

@JCQuintas JCQuintas self-assigned this Aug 29, 2024
@JCQuintas JCQuintas added docs Improvements or additions to the documentation typescript component: charts This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels Aug 29, 2024
@mui-bot
Copy link

mui-bot commented Aug 29, 2024

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

Generated by 🚫 dangerJS against f03be0a

Copy link

codspeed-hq bot commented Aug 29, 2024

CodSpeed Performance Report

Merging #14392 will not alter performance

Comparing JCQuintas:pass-all-props-to-legend (f03be0a) with master (3a4925a)

Summary

✅ 3 untouched benchmarks

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Look nice 👍

Comment on lines 72 to 76
/**
* Set to true to hide the legend.
* @default false
*/
hidden?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Make me think that this hidden and the early return null associated would better fit in the parent components to do not compute item placement if the legend is hidden

@JCQuintas JCQuintas merged commit 2cb93f4 into mui:master Sep 2, 2024
19 checks passed
@JCQuintas JCQuintas deleted the pass-all-props-to-legend branch September 2, 2024 14:48
import { DefaultizedProps } from '../models/helpers';
import { DefaultChartsLegend, LegendRendererProps } from './DefaultChartsLegend';
import { useDrawingArea } from '../hooks';
import { useSeries } from '../hooks/useSeries';
import { LegendPlacement } from './legend.types';

export type ChartsLegendPropsBase = Omit<
LegendRendererProps,
keyof LegendPlacement | 'series' | 'seriesToDisplay' | 'drawingArea'
Copy link

Choose a reason for hiding this comment

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

Was this intended to make seriesToDisplay an invalid key to BarChart.slotProps.legend ?
image

Edit: Looks like an issue is already open for this: #14561

Copy link
Member

Choose a reason for hiding this comment

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

It was intentional for the ChartsLegendPropsBase because those props are also used for color map legends

But not sure it was intentional for the slot to lose it @JCQuintas can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 It wasn't intentional, but I expected our type generating scripts to pick it up 😓

@alexfauquette

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! docs Improvements or additions to the documentation enhancement This is not a bug, nor a new feature typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants