Skip to content

Conversation

@im3dabasia
Copy link
Contributor

@im3dabasia im3dabasia commented Mar 26, 2025

What?

Discovered while working on #57862
Related comment: #57862 (comment)

Why?

This PR documents the disabled prop for ToggleGroupControlOption and adds proper TypeScript type definitions for better developer experience.

While PR #63450 added the functionality to disable individual options in ToggleGroupControl, this wasn't documented in the README or demonstrated in Storybook examples. Adding this documentation and proper TypeScript types improves discoverability and makes implementation more consistent with other components in the library.

How?

I worked on the following improvements:

  1. Added the disabled prop to the types.ts file for ToggleGroupControlOption
  2. Updated the README documentation to include information about the disabled prop
  3. Added a Storybook example demonstrating the disabled state similar to WithTooltip and WithIcons examples

These changes align with how other components like Button, FormToggle, and DropdownOption handle their disabled states.

@im3dabasia im3dabasia marked this pull request as ready for review March 26, 2025 12:56
@im3dabasia im3dabasia requested a review from ajitbohra as a code owner March 26, 2025 12:56
@github-actions
Copy link

github-actions bot commented Mar 26, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@im3dabasia
Copy link
Contributor Author

Hi @Mamaduka and @t-hamano ,

When you have a moment please review this PR. Looking forward to your feedbacks.

Copy link
Contributor

@ciampo ciampo Mar 27, 2025

Choose a reason for hiding this comment

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

I don't think we need to add the disabled type since all *Option components already inherit it from the button HTML element (example).

Although this will be an interesting factor to consider when auto-generiting the docs (cc @mirka )

Copy link
Member

Choose a reason for hiding this comment

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

By the way, there is a way to add custom filter logic to react-docgen-typescript so that any disabled props will always be visible in the Storybook props table (and thus the auto-generated readmes). Do you think that's worth doing?

IIRC, the default logic filters out standard HTML attribute props that don't have explicit descriptions, as well as prop type definitions that originate in node_modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could make exceptions for the most important / impactful attributes such as disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to add an exception for the disabled prop when generation the components doc:

Diff
diff --git a/bin/api-docs/gen-components-docs/index.mjs b/bin/api-docs/gen-components-docs/index.mjs
index 30888acf85..5455799771 100644
--- a/bin/api-docs/gen-components-docs/index.mjs
+++ b/bin/api-docs/gen-components-docs/index.mjs
@@ -19,8 +19,14 @@ const MANIFEST_GLOB = 'packages/components/src/**/docs-manifest.json';
 const OPTIONS = {
        shouldExtractLiteralValuesFromEnum: true,
        shouldRemoveUndefinedFromOptional: true,
-       propFilter: ( prop ) =>
-               prop.parent ? ! /node_modules/.test( prop.parent.fileName ) : true,
+       propFilter: ( prop ) => {
+               if ( prop.name === 'disabled' ) {
+                       return true;
+               }
+               return prop.parent
+                       ? ! /node_modules/.test( prop.parent.fileName )
+                       : true;
+       },
        savePropValueAsString: true,
 };

After that, when I ran npm run docs:components, the documentation for the FormFileUpload component was updated:

Diff
diff --git a/packages/components/src/form-file-upload/README.md b/packages/components/src/form-file-upload/README.md
index 74e6e36938..3ec885b44d 100644
--- a/packages/components/src/form-file-upload/README.md
+++ b/packages/components/src/form-file-upload/README.md
@@ -46,6 +46,11 @@ can be uploaded by the user. e.g: `image/*,video/*`.
 
 Children are passed as children of `Button`.
 
+### `disabled`
+
+ - Type: `boolean`
+ - Required: No
+
 ### `icon`
 
  - Type: `IconType`

First of all, should we consider automatically generating documentation for the ToggleGroupControl component and its subcomponents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @t-hamano , Thanks for the detailed explanation.

I have little to no experience working with automated document generation tools. Am I expected to update the PR with the above diffs you pointed out? ie. add disabled as an exception, then run this command npm run docs:components.

Copy link
Contributor

@ciampo ciampo Apr 14, 2025

Choose a reason for hiding this comment

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

Yup, you can look at docs-manifest.json files in the components package and look for related PRs (such as #68209) for inspiration.

Before auto-generating docs, you may have to re-organize the way ToggleGroupControl exports its compound components, from the current way to using Object.assign (see these docs and how Menu and Tabs do it).

Copy link
Member

Choose a reason for hiding this comment

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

We need to add the prop filter logic that @t-hamano demonstrated in the Storybook main.js as well.

For the purposes of this PR, how about we keep the manual README for this component, and deal with auto-generation as a separate task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened a new PR which focuses on automating the doc in this PR #69970. Please have a look at it and provide me feedback to improve upon it.

For the purpose of this PR. We have the disabled prop added in the README.md, Please let me know if any other change is needed from my end.

@im3dabasia
Copy link
Contributor Author

Hey @t-hamano ,

Is there any possibility we continue with this PR? on terms shared by @ciampo

For the purposes of this PR, how about we keep the manual README for this component, and deal with auto-generation as a separate task?

Let me know what are your thoughts for the same. I have raised a PR which looks after the auto-generation part #69970 But maybe for the the scope of the current issue we can add it manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components [Type] Developer Documentation Documentation for developers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants