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(props): remove jsdoc typedefs for props #641

Merged
merged 3 commits into from
Jun 22, 2021
Merged

docs(props): remove jsdoc typedefs for props #641

merged 3 commits into from
Jun 22, 2021

Conversation

varl
Copy link
Contributor

@varl varl commented Jun 22, 2021

We prefer to use React doc strings to document props, as that is what
most of the tools in the React ecosystem expects, e.g. Storybook and
React docgen.

Having JSDoc typedefs in additon means that we have two keep two
documentation styles in sync with the prop changes.

A follow-up for some components may be required to add relevant React
docstrings to props where we used to have typedefs coverage.

varl added 2 commits June 22, 2021 16:44
We prefer to use React doc strings to document props, as that is what
most of the tools in the React ecosystem expects, e.g. Storybook and
React docgen.

Having JSDoc typedefs in additon means that we have two keep two
documentation styles in sync with the prop changes.

A follow-up for some components may be required to add relevant React
docstrings to props where we used to have typedefs coverage.
We align with the way react-docgen extracts information from our
components, and that means that we can strip out our JSDoc strings. In a
lot of places they are stale, and in most cases they follow a format
that is not necessary for us.

Instead we can use a simpler format:

    /**
     * General component description.
     */
     const Component = ({ foo, bar }) => (...)

     Component.propTypes = {
        /**
         * multi-line description
         * of prop "foo"
         */
        foo: PropTypes.number,

        /** single-line description of prop "bar" */
         bar: PropTypes.number,
     }

This should be enough for most cases and tools that build on
react-docgen.
@cypress
Copy link

cypress bot commented Jun 22, 2021



Test summary

512 0 0 0


Run details

Project ui
Status Passed
Commit 11f4e7a
Started Jun 22, 2021 5:34 PM
Ended Jun 22, 2021 5:46 PM
Duration 12:06 💡
OS Linux Ubuntu - 20.04
Browser Electron 89

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@mediremi mediremi left a comment

Choose a reason for hiding this comment

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

This just reduced our maintenance burden by ~:infinity:!
Thank you 😄

I think we might also want to remove this:

/**
* @prop {string[]} [idsThatShouldBeReloaded]
* All units with ids (not paths!) provided
* to "idsThatShouldBeReloaded" will be reloaded
* In order to reload an id twice, the array must be changed
* while keeping the id to reload in the array
*
* NOTE: This is currently not working due to a limitation
* of the data engine (we can't force specific resource to reload,
* we'd have to reload the sibling nodes currently as well)
*/
//idsThatShouldBeReloaded: propTypes.arrayOf(orgUnitIdPropType),

@varl
Copy link
Contributor Author

varl commented Jun 22, 2021

I think we might also want to remove this:

Good catch !

@varl
Copy link
Contributor Author

varl commented Jun 22, 2021

The Transfer, FileInput, and OrganisationUnitTree uses JSDoc strings for helper functions, and I have left those because they make the functions a lot easier to understand. Note that these are not Components, so using JSDoc here is alright.

@varl varl merged commit 01eea9b into master Jun 22, 2021
@varl varl deleted the jsdoc branch June 22, 2021 17:55
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 6.10.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants