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

[docs] Add missing API docs #2167

Merged
merged 7 commits into from
Jul 22, 2021
Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Jul 18, 2021

Part of #2146 to add doc of onCellFocusOut prop.

Also added some other props from GridOptions which were missing.

Preview:
XGrid: https://deploy-preview-2167--material-ui-x.netlify.app/api/data-grid/x-grid/
DataGrid: https://deploy-preview-2167--material-ui-x.netlify.app/api/data-grid/data-grid/

@ZeeshanTamboli ZeeshanTamboli added docs Improvements or additions to the documentation component: data grid This is the name of the generic UI component, not the React module! labels Jul 18, 2021
@ZeeshanTamboli ZeeshanTamboli requested a review from a team July 18, 2021 07:22
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. It's easy to have this docs outdated, the sooner we upgrade the docs infra to v5, so we can use the docs generator of the main repository, the better.

@@ -143,8 +143,8 @@ export interface GridOptions {
editRowsModel?: GridEditRowsModel;
/**
* Filtering can be processed on the server or client-side.
* Set it to 'client' if you would like to handle filtering on the client-side.
Copy link
Member

Choose a reason for hiding this comment

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

This still seem relevant, why remove it?

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Jul 18, 2021

Choose a reason for hiding this comment

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

The default is client, so I added @default. I removed it from here as I thought we need not explicitly set it to client because it is default.

Copy link
Member

Choose a reason for hiding this comment

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

In the core, these comments are used in boolean props. There, the default value goes to @default and the comment only mentions what the non-default value does.

image

I don't have any preference here, as long as we keep consistency: remove from all other feature mode props or keep this line.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, we can move slower: only add the default mention, it's already an improvement.

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 already have added the @default tag.

docs/pages/api-docs/data-grid/x-grid.md Outdated Show resolved Hide resolved
@@ -84,6 +89,7 @@ import { DataGrid } from '@material-ui/data-grid';
| <span class="prop-name">onRowLeave</span> | <span class="prop-type">(param: GridRowParams, event: React.MouseEvent) => void</span> | | Callback fired when a mouse leave event comes from a row container element. |
| <span class="prop-name">onSelectionModelChange</span> | <span class="prop-type">(param: GridSelectionModelChangeParams) => void</span> | | Callback fired when the selection state of one or multiple rows changes. |
| <span class="prop-name">onSortModelChange</span> | <span class="prop-type">(param: GridSortModelParams) => void</span> | | Callback fired when the sort model changes before a column is sorted. |
| <span class="prop-name">onStateChange</span> | <span class="prop-type">(params: any) => void</span> | | Callback fired when the state of the grid is updated. |
Copy link
Member

Choose a reason for hiding this comment

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

Private API

Copy link
Member

@m4theushw m4theushw Jul 19, 2021

Choose a reason for hiding this comment

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

If it's private we should consider removing this prop. It's not used internally.

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Jul 20, 2021

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about the prop. We don't use it internally. XGrid's users could still get state updates by subscribing to GRID_STATE_CHANGE.

Copy link
Member

@oliviertassinari oliviertassinari Jul 20, 2021

Choose a reason for hiding this comment

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

@m4theushw We have mentioned it in some of the issues. I think that we should consider it experimental at this point (not documented). It's unclear what problem it solves.

Copy link
Member Author

Choose a reason for hiding this comment

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

So based on this conversation there are 2 things:

  1. Remove the doc and the prop both.
  2. Only remove the doc.

What do we do?

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, I removed this API from docs.

@oliviertassinari oliviertassinari requested a review from a team July 18, 2021 09:11
docs/pages/api-docs/data-grid/data-grid.md Outdated Show resolved Hide resolved
docs/pages/api-docs/data-grid/data-grid.md Outdated Show resolved Hide resolved
@@ -84,6 +89,7 @@ import { DataGrid } from '@material-ui/data-grid';
| <span class="prop-name">onRowLeave</span> | <span class="prop-type">(param: GridRowParams, event: React.MouseEvent) => void</span> | | Callback fired when a mouse leave event comes from a row container element. |
| <span class="prop-name">onSelectionModelChange</span> | <span class="prop-type">(param: GridSelectionModelChangeParams) => void</span> | | Callback fired when the selection state of one or multiple rows changes. |
| <span class="prop-name">onSortModelChange</span> | <span class="prop-type">(param: GridSortModelParams) => void</span> | | Callback fired when the sort model changes before a column is sorted. |
| <span class="prop-name">onStateChange</span> | <span class="prop-type">(params: any) => void</span> | | Callback fired when the state of the grid is updated. |
Copy link
Member

@m4theushw m4theushw Jul 19, 2021

Choose a reason for hiding this comment

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

If it's private we should consider removing this prop. It's not used internally.

@@ -143,8 +143,8 @@ export interface GridOptions {
editRowsModel?: GridEditRowsModel;
/**
* Filtering can be processed on the server or client-side.
* Set it to 'client' if you would like to handle filtering on the client-side.
Copy link
Member

Choose a reason for hiding this comment

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

In the core, these comments are used in boolean props. There, the default value goes to @default and the comment only mentions what the non-default value does.

image

I don't have any preference here, as long as we keep consistency: remove from all other feature mode props or keep this line.

@ZeeshanTamboli
Copy link
Member Author

I have also updated the onSelectionModelChange API doc in commit 63432bd

@ZeeshanTamboli
Copy link
Member Author

I am a bit confused here that the https://material-ui.com/api/data-grid/data-grid/ is still showing (param: GridSortModelParams) => void for onSortModelChange.
Capture 2

While on master HEAD it has been updated: https://github.com/mui-org/material-ui-x/blob/master/docs/pages/api-docs/data-grid/data-grid.md.

Did we release the docs?

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Good work. 👍

@oliviertassinari oliviertassinari removed their request for review July 22, 2021 22:37
@m4theushw m4theushw merged commit a1d05f2 into mui:master Jul 22, 2021
@m4theushw
Copy link
Member

Did we release the docs?

We had a problem during the deploy. Now it's updated.

@ZeeshanTamboli ZeeshanTamboli deleted the add-missing-docs branch July 23, 2021 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid 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.

3 participants