Skip to content

Conversation

@albertchae
Copy link

@albertchae albertchae commented Jul 22, 2024

The type shows it is the same as Modal's onClose
https://github.com/mui/material-ui/blob/v4.x/packages/material-ui/src/Drawer/Drawer.d.ts#L31-L36

So I copied the documentation line about reason from Modal.d.ts, ran yarn proptypes and yarn docs:api

This generates almost the same doc as modal.md, but there is an additional line in the modal documentation that wasn't present in Modal.d.ts 🤷
https://github.com/mui/material-ui/blob/v4.x/docs/pages/api-docs/modal.md?plain=1#L54

The reason parameter can optionally be used to control the response to onClose

This is already correctly documented in v5

One caveat is that I am targeting the v4.x branch since this documentation needs to be backported there

@zannager zannager added docs Improvements or additions to the documentation. scope: drawer Changes related to the drawer. labels Jul 23, 2024
@zannager zannager requested a review from DiegoAndai July 23, 2024 12:40
@zanivan zanivan changed the title Document Drawer onClose reason in v4 [material-ui][docs] Document Drawer onClose reason in v4 Jul 29, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 26, 2024

Hey @albertchae, thanks for working on this, and sorry for the late review.

I agree with the change. This .md content is generated from Drawer.d.ts, so may I ask you to modify that instead (You can copy it from modal as well) and run yarn proptypes and then yarn docs:api? That should modify the docs content accordingly.

@albertchae albertchae force-pushed the v4-drawer-onClose-reason branch from 70b4867 to 6f2edfa Compare August 27, 2024 17:48
@albertchae
Copy link
Author

Thanks @DiegoAndai , I followed those steps and pushed my changes with updated PR description. One slightly weird thing I noticed compared to when I manually copied from modal.md is this:

This generates almost the same doc as modal.md, but there is an additional line in the modal documentation that wasn't present in Modal.d.ts 🤷
https://github.com/mui/material-ui/blob/v4.x/docs/pages/api-docs/modal.md?plain=1#L54

The reason parameter can optionally be used to control the response to onClose

@aarongarciah aarongarciah requested review from DiegoAndai and removed request for DiegoAndai December 25, 2024 11:14
@DiegoAndai
Copy link
Member

Hey @albertchae, sorry for the delay. If you're still interested in merging this change, may I ask you to work on the failing CI tests?

The type shows it is the same as Modal's onClose
https://github.com/mui/material-ui/blob/v4.x/packages/material-ui/src/Drawer/Drawer.d.ts#L31-L36

So I copied the documentation line about reason from `Modal.d.ts`, ran `yarn proptypes` and
`yarn docs:api`

This generates almost the same doc as `modal.md`, but there is an
additional line in the modal documentation that wasn't present in
`Modal.d.ts` ¯\_(ツ)_/¯
https://github.com/mui/material-ui/blob/v4.x/docs/pages/api-docs/modal.md?plain=1#L54
"The `reason` parameter can optionally be used to control the response to `onClose`"

This is already correctly documented in v5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: drawer Changes related to the drawer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants