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

Components: add missing documentation #33263

Open
6 of 32 tasks
Tracked by #34304
ciampo opened this issue Jul 7, 2021 · 17 comments
Open
6 of 32 tasks
Tracked by #34304

Components: add missing documentation #33263

ciampo opened this issue Jul 7, 2021 · 17 comments
Assignees
Labels
Developer Experience Ideas about improving block and theme developer experience [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Developer Documentation Documentation for developers

Comments

@ciampo
Copy link
Contributor

ciampo commented Jul 7, 2021

What problem does this address?

As explained in #33111, some components in the @wordpress/components package don't have associated JSDocs / README's, or if they do, they are not complete with proper documentation and examples.

What is your proposed solution?

Add JSDocs / README documentation for:

  • AlignmentMatrixControl
  • Card
  • CircularOptionPicker
  • ClipboardButton
  • ColorControl
  • ColorEdit
  • ColorIndicator
  • ColorListPicker
  • ColorPicker
  • ControlGroup
  • ControlLabel
  • Composite
  • CustomGradientPicker
  • Dashicon
  • Disclosure
  • Divider
  • ExternalLink
  • Flex
  • Flyout
  • FooterMessageControl
  • FormGroup
  • GradientPicker
  • ItemGroup
  • Radio
  • ResponsiveWrapper
  • Sandbox
  • Shortcut
  • StyleProvider
  • Swatch
  • Tip
  • ToolbarContext
  • ToolPanel

Internal components like View should not have a README

@ryanwelcher
Copy link
Contributor

@ciampo I'm not sure if there is an existing ticket for this but some of the components README don't have full documentation. For example AlignmentMatrixControl has only example code.

I'm happy to create a new ticket and reference it in #34304

@ciampo
Copy link
Contributor Author

ciampo commented Sep 3, 2021

@ciampo I'm not sure if there is an existing ticket for this but some of the components README don't have full documentation. For example AlignmentMatrixControl has only example code.

I'm happy to create a new ticket and reference it in #34304

Hey @ryanwelcher , thank you for reaching out — your help would surely be appreciated!

Feel free to use this issue (instead of a new ticker) to track components that need better documentation (for example, following your advice, I've just added AlignmentMatrixControl to the list!)

Let me know if you ever want to work on improving the documentation for some of these components — I can definitely help you with those with references, advice, and PR reviews!

@ryanwelcher
Copy link
Contributor

@ciampo sounds great! I'll go through the rest of the Components, add any that need updates here and start on some PRs!

@ciampo
Copy link
Contributor Author

ciampo commented Sep 3, 2021

add any that need updates here and start on some PRs!

Excellent! Please keep me in the loop with those updates / PRs!

@ryanwelcher
Copy link
Contributor

@ciampo wondering if we have an example of documenting when props are passed to an underlying component? For example, in #34711, it would be great to have a line to the effect of "any other props are passed down the underlying <Button/>" - just looking to standardize that in some way.

@ciampo
Copy link
Contributor Author

ciampo commented Sep 14, 2021

@ciampo wondering if we have an example of documenting when props are passed to an underlying component? For example, in #34711, it would be great to have a line to the effect of "any other props are passed down the underlying <Button/>" - just looking to standardize that in some way.

Hey @ryanwelcher, while there isn't an official "rule" yet on how to do this, the Card component could be a good example to follow:

### Inherited props
`Card` also inherits all of the [`Surface` props](/packages/components/src/ui/surface/README.md#props).

@ryanwelcher
Copy link
Contributor

Is there value to adding ES5 examples to each component? Most of the documentation in the developer handbooks have ES5 and ESNext examples and it might be a nice addition here as well.

@ciampo
Copy link
Contributor Author

ciampo commented Oct 15, 2021

Is there value to adding ES5 examples to each component? Most of the documentation in the developer handbooks have ES5 and ESNext examples and it might be a nice addition here as well.

I don't think so, ES6 syntax is now supported in all evergreen browsers — although I believe @gziolo is the better person to answer this

@mkaz
Copy link
Member

mkaz commented Oct 15, 2021

I agree, I think having two examples is more confusing and all the code in Gutenberg is in ES6/JSX syntax so the quicker we can get developers to that format the easier it will be for them to look at example code across Gutenberg and other sites.

Most React tutorials and other places all use a JSX syntax, so its beneficial in the long run to learn it.

Here's a reference in documentation about ES5/ESNext and a brief discussion about removing ES5 altogether

@sybrew
Copy link

sybrew commented Oct 15, 2021

Removing ES5 in favor of ES6: I'm OK with that. Great!

Removing ES5 in favor of JSX only: No, please, I don't want to build my code; not all of it pertains to Gutenberg.

Not every one that interacts with Gutenberg creates blocks. This project is bigger than anyone dares to fantom.

@mkaz
Copy link
Member

mkaz commented Oct 16, 2021

@sybrew No functionality would be removed, just a matter of documentation.

The debate isn't really ES5 vs. ES6, but do we include JSX and non-JSX examples which doubles the requirements for documentation which is already thin. Plus it requires more effort for the non-JSX documentation considering most of the code for Gutenberg is not written that way.

@sybrew
Copy link

sybrew commented Oct 16, 2021

Thank you for following up. Yes, I was speaking about the documentation. If it so happens that the code works on both ES/[1-9][0-9]*/ and JSX, then I think it'd be helpful to annotate that. If the code needs different grammar for ES vs JSX, then I think we should still have the option to choose one over the other.

Providing an option would give us the buttons on the doc pages: ES or JSX (2 buttons) and ES and JSX (1 button). Akin to what you can see in the image below:

image

My suggestion: Make both buttons green when it's ES and JSX. Developers only slightly familiar with the docs should recognise what it conveys.

I think that shall also help onboarding and further acceptance of the project, especially by those who only need to interact with the framework once and do not have the resources or willingness to figure out how to build with JSX.

I don't think time should be an argument of the removal of ES-docs. How vast do you want the public API to be, anyway? A few hundred documented examples should amount to a few hundred minutes of extra work documenting ES next to JSX... once. Maybe a couple of more maintaining the docs, but that's (5) for the future.

@mkaz
Copy link
Member

mkaz commented Oct 16, 2021

To clear it up, we probably should to switch the toggle to "JSX" and "Plain" then it shows one needs a build step and the other is just plain JavaScript that could be run in the browser.

I agree, we should not remove any existing documentation or examples. To me the question is do we have to have a "Plain" example alongside a "JSX" example to merge in new documentation. My thought is no, it shouldn't be required to merge, it is a nice-to-have and I'd rather get things documented and have something rather than nothing. As additional examples can be added later.

@gziolo
Copy link
Member

gziolo commented Oct 18, 2021

To clear it up, we probably should to switch the toggle to "JSX" and "Plain" then it shows one needs a build step and the other is just plain JavaScript that could be run in the browser.

Now that we no longer support IE11, it is no longer necessary to have ES5 examples included. ES6+ or ES2015+ has also been around for more than 6 years, so it’s widely popularized, and it's about time to focus on it.

I agree that we should decide instead whether to include an optional non-JSX version when dealing with React components.

@gziolo gziolo added the Developer Experience Ideas about improving block and theme developer experience label Oct 18, 2021
@evrpress
Copy link
Contributor

Hi all!

I've added a readme to GradientPicker (#37614) as well to the ColorIndicator (#37638). Let me know if this is enough @ciampo

@ciampo
Copy link
Contributor Author

ciampo commented Jan 19, 2022

Hi all!

I've added a readme to GradientPicker (#37614) as well to the ColorIndicator (#37638). Let me know if this is enough @ciampo

Hey @evrpress , we really appreciate your help here! I marked those 2 components as done in this issue's description :) In case you feel like opening more of these PRs, please go ahead (and feel free to ping me / add me a reviewer!)

@evrpress
Copy link
Contributor

The one for the DashIcon (#37641) is still open

@ryanwelcher ryanwelcher self-assigned this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

No branches or pull requests

7 participants