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

Composite: improve Storybook examples and clean up prop docs #64397

Merged
merged 15 commits into from
Aug 9, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 9, 2024

What?

Follow-up to #63564

Improve Storybook examples for the Composite component.

Why?

Better documentation for Composite

How?

  • add interactive controls
  • add more examples on top of the existing grid example (default, groups)

Testing Instructions

Open Storybook, go to the Composite component:

  • make sure interactive controls behave as expected
  • make sure the newly added examples make sense and offer a more complete showcase of the component's offering.

This PR doesn't include runtime changes to the actual component.

Copy link

github-actions bot commented Aug 9, 2024

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: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

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

@ciampo ciampo self-assigned this Aug 9, 2024
@ciampo ciampo added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components labels Aug 9, 2024
@ciampo ciampo requested a review from a team August 9, 2024 11:22
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

This is a really nice improvement! 🚀

packages/components/src/composite/stories/index.story.tsx Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a better description for this too? Suggestion:

diff --git a/packages/components/src/composite/stories/utils.tsx b/packages/components/src/composite/stories/utils.tsx
index febbffd00b..8ce0410bf7 100644
--- a/packages/components/src/composite/stories/utils.tsx
+++ b/packages/components/src/composite/stories/utils.tsx
@@ -9,7 +9,9 @@ import type { StoryContext } from '@storybook/react';
 import type { CompositeStoreProps } from '../types';
 
 /**
- * Renders a composite widget.
+ * Renders a widget based on the WAI-ARIA [`composite`](https://w3c.github.io/aria/#composite) role,
+ * which provides a single tab stop on the page and arrow key navigation through the
+ * focusable descendants.
  *
  * ```jsx
  * const store = useCompositeStore();

Also a few docs nits I noticed (sorry to piggyback):

  • The JSDoc/README snippets need to include the import statement (import { Composite, useCompositeStore } from '@wordpress/components';).
  • The README contains a link to the Ariakit docs, which we should remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points! I went ahead, implemented your suggestions:

After that, I released that the prop description that came via Ariakit also referenced the ariakit website, other ariakit components / examples, or even other Composite props that we are not exposing. And therefore, I pushed a few more changes:

  • instead of Pick-ing prop types, I added them explicitly (still re-using the original type) so that I could re-write the prop description in the JSDoc. In the process, I also updated Storybook and README with the updated JSDocs (which are not much more complete and closer to the original, but without ariakit references)(ad9fbb4 and 929fb4c)
  • For correctness, I added explicit default values in our implementation to make sure that our runtime code is true to the default values that we declare in the docs (32dd868)

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks, @ciampo!

However, does it work as expected? When I navigate to the specific stories, the controls don't seem to work:

Screen.Recording.2024-08-09.at.17.34.03.mov

Am I missing something perhaps?

@ciampo
Copy link
Contributor Author

ciampo commented Aug 9, 2024

Am I missing something perhaps?

@tyxla :

  • The orientation prop changes the keyboard keys that are used to navigate across composite items (left/right for horizontal, up/down for vertical)
  • The rtl prop inverts what prev and next do (so, for example, I'd expect the result of pressing the left/right arrow keys to be inverted when the prop is true and the orientation is 'horizontal')

They should not affect the component visually. That should be reflected in the props' description

@tyxla
Copy link
Member

tyxla commented Aug 9, 2024

🤦 of course, that makes a lot of sense!

Tests well and as expected ✅ Thanks!

ciampo added 6 commits August 9, 2024 16:51
Instead of using Ariakit's definitions and descriptions, we use our own
version, which a copy or Ariakit's without any references to Ariakit, its
examples, or any other props that we don't expose.
Along the same fashion as the previous commit, setting explicit default
values will give us more control when propagating ariakit changes, and
will allows to stay true to our first-part props docs.
@ciampo ciampo force-pushed the feat/stabilize-composite/storybook branch from 33c485c to ad9fbb4 Compare August 9, 2024 15:46
@ciampo ciampo changed the title Composite: improve Storybook examples Composite: improve Storybook examples and clean up prop docs Aug 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

Flaky tests detected in 162fd12.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10322121415
📝 Reported issues:

@ciampo ciampo enabled auto-merge (squash) August 9, 2024 16:22
@ciampo ciampo merged commit b4ba75f into trunk Aug 9, 2024
61 checks passed
@ciampo ciampo deleted the feat/stabilize-composite/storybook branch August 9, 2024 16:31
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 9, 2024
getdave pushed a commit that referenced this pull request Aug 14, 2024
* More clear tranform function comments

* Add interactive controls

* Add simpler default Storybook example

* Improve `activeId`'s prop description

* Add Groups example

* CHANGELOG

* Removed actions config in Storybook

* Better composite description

* Remove direct references to Ariakit's docs in JSDocs and README

* Add import from `@wordpress/components` in all code snippets

* useCompositeStore: update prop docs by using first-party docs

Instead of using Ariakit's definitions and descriptions, we use our own
version, which a copy or Ariakit's without any references to Ariakit, its
examples, or any other props that we don't expose.

* useCompositeStore: set explicit default values

Along the same fashion as the previous commit, setting explicit default
values will give us more control when propagating ariakit changes, and
will allows to stay true to our first-part props docs.

* Remove unnecessary space

* Provide first-party prop descriptions also for other composite components

* Add default value for store props to avoid errors while destructuring

---

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
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.

3 participants