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

Standardize & Simplify Component Interface Format #104

Open
alexgetty opened this issue Jan 28, 2021 · 6 comments
Open

Standardize & Simplify Component Interface Format #104

alexgetty opened this issue Jan 28, 2021 · 6 comments
Labels
low priority There isn't an immediate use case at this time.

Comments

@alexgetty
Copy link
Contributor

Some component interfaces have robust inline documentation (like atoms/images/image.tsx) while others do not (like atoms/anchors/anchor.tsx). I feel like typescript interfaces are pretty self-documenting as far as types go, and the documentation in components like Image are redundant and overly verbose by adding 6 new comment lines for every 1 prop line, especially for interfaces with more than a few props. Unless there is an argument for keeping them, I propose we remove this redundant inline documentation. If we do keep it, it should be added to all component interfaces.

Screen Shot 2021-01-28 at 10 37 19 AM
Screen Shot 2021-01-28 at 10 37 28 AM

@brandongregoryscott
Copy link
Contributor

Going to have to disagree here - there are certainly cases where behavior cannot be inferred just by prop name, and extra commentary via a JSDoc can really shed some light. Especially if this component library has external adopters - we are more familiar with the codebase and prop naming conventions, but others will not be.

Sure, some of the lines in those JSDoc comments are redundant (such as types, or the @memberof line), those can probably be stripped out as we come across them. A lot of them are probably generated from IDE extensions.

For the time being, I'd advise against a hard rule for adding documentation comments to every single prop or removing documentation comments from every single prop. They're inconsistent because they were pulled from a client project, and there was no tool enforcing a pattern one way or the other. We can definitely look into tooling to add build warnings for missing documentation fields, but based on similar tooling for our C# packages, it's going to take a while to backfill them, and I'm not sure what value it provides just yet.

@brandongregoryscott brandongregoryscott added the low priority There isn't an immediate use case at this time. label Jan 28, 2021
@alexgetty
Copy link
Contributor Author

Is there a way to include a description of the prop for JSDoc without adding the redundant @type and @memberof lines for each one? I could get behind that for sure.

@brandongregoryscott
Copy link
Contributor

brandongregoryscott commented Jan 28, 2021

Is there a way to include a description of the prop for JSDoc without adding the redundant @type and @memberof lines for each one? I could get behind that for sure.

Absolutely. I'm sure there are some in there that don't have them, but a lot of them do, and it's probably from me, admittedly. (The Document This extension for VS Code is what I use) By default it adds the @type and @memberof lines, but I have since updated my settings to exclude those. The generated typescript definition files should take care of the type/member inference for consumers, so there's no real benefit there.

@alexgetty
Copy link
Contributor Author

Excellent. I'll check out Document This and perhaps once I can resolve some of the backlog of PRs already in the works, I'll go through and clean up the existing interfaces so we start with a solid foundation for future components.

@brandongregoryscott
Copy link
Contributor

@alexgetty I had recently done a pass on a client project that also had them all over the place - the bulk of them can be removed with a quick regular expression.

@alexgetty
Copy link
Contributor Author

alexgetty commented Mar 11, 2021

Regular expressions you say? backs away slowly

If you have a chance at some point and could share your process for that, I'd be happy to take a stab at it for this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority There isn't an immediate use case at this time.
Projects
None yet
Development

No branches or pull requests

2 participants