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] Improve properties JSDoc #10931

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Conversation

alexfauquette
Copy link
Member

The chart's API pages were somewhat empty. It's just because most of the props don't have JSDocs 🫣

I'm adding some of them. I've splited the PR by commit with JSdocs, and commits with scripts

@mui-bot
Copy link

mui-bot commented Nov 7, 2023

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

Generated by 🚫 dangerJS against 34a41d6

@alexfauquette alexfauquette added docs Improvements or additions to the documentation component: charts This is the name of the generic UI component, not the React module! labels Nov 7, 2023
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 improvement! 👏
WDYT about changing the PR title to [charts] Improve properties JSDoc?
It seems that the PR targets props that are also only dev facing, not only the ones that end up in API docs. 🤔

Complete nitpick: Should this PR maybe target next as the source of truth and then be cherry-picked to master?

height: number;
viewBox?: ViewBox;
className?: string;
title?: string;
desc?: string;
sx?: SxProps<Theme>;
children?: React.ReactNode;
/**
* If true, the charts will not listen to the mouse move event.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If true, the charts will not listen to the mouse move event.
* If `true`, the charts will not listen to the mouse move event.

height: number;
viewBox?: ViewBox;
className?: string;
title?: string;
desc?: string;
sx?: SxProps<Theme>;
children?: React.ReactNode;
/**
* If true, the charts will not listen to the mouse move event.
* It might breaks interactive features, but save some computation power.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* It might breaks interactive features, but save some computation power.
* It might break interactive features but will improve performance.

@@ -55,6 +55,13 @@ export interface LineChartProps
Omit<ChartsAxisProps, 'slots' | 'slotProps'> {
series: MakeOptional<LineSeriesType, 'type'>[];
tooltip?: ChartsTooltipProps;
/**
* Object `{ x, y }` that defines how the charts highlight the mouse position along x and y axes.
* The two proerties accept the next values:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The two proerties accept the next values:
* The two properties accept the following values:

@@ -55,6 +55,13 @@ export interface LineChartProps
Omit<ChartsAxisProps, 'slots' | 'slotProps'> {
series: MakeOptional<LineSeriesType, 'type'>[];
tooltip?: ChartsTooltipProps;
/**
* Object `{ x, y }` that defines how the charts highlight the mouse position along x and y axes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Object `{ x, y }` that defines how the charts highlight the mouse position along x and y axes.
* Object `{ x, y }` that defines how the charts highlight the mouse position along the x and y axes.

* The two proerties accept the next values:
* - 'none': display nothing.
* - 'line': display a line at the current mouse position.
* - 'band': display a band at the current mouse position. Only available with band scale.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* - 'band': display a band at the current mouse position. Only available with band scale.
* - 'band': display a band at the current mouse position. Only available with band scale.

@@ -8,5 +8,11 @@ export interface CardinalDirections<T> {
export type LayoutConfig = {
width: number;
height: number;
/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* It's used for leving space for extra information wuch as axes, or legend.
* It's used for leaving space for extra information such as axes, or legend.

/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
* Accepts an object with some of the following properties: `top`, `bottom`, `left`, and `right`.

yAxisKey?: string;
};

export type StackableSeriesType = {
/**
* The key that identify the stacking group.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The key that identify the stacking group.
* The key that identifies the stacking group.

stack?: string;
/**
* Defines hos staked series handle negative values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Defines hos staked series handle negative values.
* Defines how stacked series handles negative values.

stackOffset?: StackOffsetType;
/**
* The order in which series of the same staking group are staked together.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The order in which series of the same staking group are staked together.
* The order in which series of the same staking group are staked together.

@alexfauquette alexfauquette changed the title [docs] Improve docs API coverage [charts] Improve properties JSDoc Nov 8, 2023
@alexfauquette
Copy link
Member Author

Complete nitpick: Should this PR maybe target next as the source of truth and then be cherry-picked to master?

As long as there is no breaking change, I backport everything.

Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

SRY for all the comments. A ton of them are duplicates, but midway through I couldn't stop anymore! 🙇🏼 🙇🏼 🙇🏼

stackOffset?: StackOffsetType;
/**
* The order in which series of the same staking group are staked together.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The order in which series of the same staking group are staked together.
* The order in which series' of the same group are stacked together.

stack?: string;
/**
* Defines hos staked series handle negative values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Defines hos staked series handle negative values.
* Defines how stacked series handle negative values.

yAxisKey?: string;
};

export type StackableSeriesType = {
/**
* The key that identify the stacking group.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The key that identify the stacking group.
* The key that identifies the stacking group.

Comment on lines 11 to 16
/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
* @default object depends on the charts type.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
* @default object depends on the charts type.
*/
/**
* The margin between the SVG and the drawing area.
* It's used for leaving some space for extra information such as the x- and y-axis or legend.
* Accepts an object with the optional properties: `top`, `bottom`, `left`, and `right`.
* @default object Depends on the charts type.
*/

@@ -202,6 +216,9 @@ export type AxisDefaultized<S extends ScaleName = ScaleName, V = any> = Omit<
'scaleType'
> &
AxisScaleConfig[S] & {
/**
* An indication of the number of ticks expected.
Copy link
Member

Choose a reason for hiding this comment

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

Not to sure about this indication.

Could be either ...

Suggested change
* An indication of the number of ticks expected.
* An indication of the expected number of ticks.

or ...

Suggested change
* An indication of the number of ticks expected.
* The expected number of ticks.

Comment on lines 26 to 30
/**
* If true, the charts will not listen to the mouse move event.
* It might breaks interactive features, but save some computation power.
* @default false
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* If true, the charts will not listen to the mouse move event.
* It might breaks interactive features, but save some computation power.
* @default false
*/
/**
* If `true`, the charts will not listen to the mouse move event.
* It might break interactive features, but is improving performance.
* @default false
*/

Comment on lines 233 to 237
/**
* If true, the charts will not listen to the mouse move event.
* It might breaks interactive features, but save some computation power.
* @default false
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* If true, the charts will not listen to the mouse move event.
* It might breaks interactive features, but save some computation power.
* @default false
*/
/**
* If `true`, the charts will not listen to the mouse move event.
* It might break interactive features, but is improving performance.
* @default false
*/

Comment on lines 293 to 298
/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
* @default object depends on the charts type.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* The margin between the SVG and the drawing area.
* It's used for leving space for extra information wuch as axes, or legend.
* Accept and object with some of the next properties: `top`, `bottom`, `left`, and `right`.
* @default object depends on the charts type.
*/
/**
* The margin between the SVG and the drawing area.
* It's used for leaving some space for extra information such as the x- and y-axis or legend.
* Accepts an object with the optional properties: `top`, `bottom`, `left`, and `right`.
* @default object Depends on the charts type.
*/

packages/x-charts/src/BarChart/BarChart.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/BarChart/BarChart.tsx Outdated Show resolved Hide resolved
@alexfauquette
Copy link
Member Author

Thanks for all your comments. I think I nearly integrated each of them 👍

@alexfauquette alexfauquette merged commit a0be8ca into mui:master Nov 8, 2023
5 checks passed
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Nov 8, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants