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: stabilize new ariakit implementation #63564

Merged
merged 39 commits into from
Aug 9, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jul 15, 2024

What?

Extracted from #63364

Part of #58850

Export the new ariakit-based Composite component from the @wordpress/components package.

Why?

The component has been available via private APIs for a while without causing issues. It's time to stabilize it!

How?

  • Renamed the component's exports following the compound components naming convention;
  • Updated all internal references to the component package, including private APIs;
  • Exported the new component from the @wordpress/components package
  • Updated storybook files
  • Added documentation where needed

Note: from the point of view of a consumer of @wordpress/components, this PR only adds new APIs and shouldn't require any code changes.

Follow-ups

Testing Instructions

No changes were made to how the component works, only to how it's exported / imported.

Smoke test that usages of the component both in Storybook and in the editor work as on trunk.

Reviewing commit-by-commit may help.

@@ -14,7 +14,7 @@ import type { Icon } from '@wordpress/icons';
import type { ButtonAsButtonProps } from '../button/types';
import type { DropdownProps } from '../dropdown/types';
import type { WordPressComponentProps } from '../context';
import type { CompositeStore } from '../composite/v2';
import type { Store as CompositeStore } from '../composite';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above for types — I went for Store as I expect consumers to consume this API as Composite.Store (or by aliasing the import like in this case)

@@ -15,7 +15,8 @@ import {
import { UseCompositeStatePlaceholder, transform } from './utils';

const meta: Meta< typeof UseCompositeStatePlaceholder > = {
title: 'Components/Composite',
// TODO: should we keep this story around? If so, how should we call it?
title: 'Components/Composite/Legacy',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably @mirka is best-suited for the above TODO question. Also, see other comment on the current implementation's Storybook title

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep it like we keep other deprecated component stories (like Navigation)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should keep it as long as it's available in the package. As for how, my recommendation would be like this:

diff --git a/packages/components/src/composite/legacy/stories/index.story.tsx b/packages/components/src/composite/legacy/stories/index.story.tsx
index 08eebc65ed..d3d3205afe 100644
--- a/packages/components/src/composite/legacy/stories/index.story.tsx
+++ b/packages/components/src/composite/legacy/stories/index.story.tsx
@@ -15,8 +15,8 @@ import {
 import { UseCompositeStatePlaceholder, transform } from './utils';
 
 const meta: Meta< typeof UseCompositeStatePlaceholder > = {
-	// TODO: should we keep this story around? If so, how should we call it?
-	title: 'Components/Composite/Legacy',
+	title: 'Components (Deprecated)/Composite (Unstable)',
+	id: 'components-composite-unstable',
 	component: UseCompositeStatePlaceholder,
 	subcomponents: {
 		// @ts-expect-error Storybook doesn't like overloaded exports as subcomponents
diff --git a/packages/components/src/composite/legacy/stories/utils.tsx b/packages/components/src/composite/legacy/stories/utils.tsx
index 06edd34863..2fb51c845f 100644
--- a/packages/components/src/composite/legacy/stories/utils.tsx
+++ b/packages/components/src/composite/legacy/stories/utils.tsx
@@ -8,6 +8,25 @@ import type { StoryContext } from '@storybook/react';
  */
 import type { LegacyStateOptions } from '..';
 
+/**
+ * Renders a composite widget.
+ *
+ * This unstable component is deprecated. Use `Composite` instead.
+ *
+ * ```jsx
+ * import {
+ * 	__unstableUseCompositeState as useCompositeState,
+ * 	__unstableComposite as Composite,
+ * 	__unstableCompositeItem as CompositeItem,
+ * } from '@wordpress/components';
+ *
+ * const state = useCompositeState();
+ * <Composite state={ state }>
+ * 	<CompositeItem>Item 1</CompositeItem>
+ * 	<CompositeItem>Item 2</CompositeItem>
+ * </Composite>;
+ * ```
+ */
 export function UseCompositeStatePlaceholder( props: LegacyStateOptions ) {
 	return (
 		<dl>

The important points being:

  • It needs to be clear that we're referring to the __unstable version, and that it's deprecated.
  • Usually we want to ensure that we don't break permalinks, but in this specific case I think we would cause the less damage if we just made a clean break and changed the URLs so it reflects the new situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, done in b3e58bc

@ciampo ciampo mentioned this pull request Jul 15, 2024
3 tasks
@ciampo ciampo self-assigned this Jul 15, 2024
@ciampo ciampo added the [Package] Components /packages/components label Jul 15, 2024
@ciampo ciampo requested a review from a team July 15, 2024 13:34
@ciampo ciampo added the [Type] New API New API to be used by plugin developers or package users. label Jul 15, 2024
@ciampo ciampo marked this pull request as ready for review July 15, 2024 13:34
@ciampo ciampo requested a review from ajitbohra as a code owner July 15, 2024 13:34
Copy link

github-actions bot commented Jul 15, 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 <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

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

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 for working on this one @ciampo! Excited about stabilizing the Ariakit version of Composite!

This looks and works great overall!

Left some comments and questions, some might be naive, so take them with a grain of salt 😉

packages/components/src/composite/legacy/index.tsx Outdated Show resolved Hide resolved
packages/dataviews/src/layouts/list/index.tsx Outdated Show resolved Hide resolved
packages/components/src/composite/README.md Show resolved Hide resolved
packages/components/src/composite/stories/utils.tsx Outdated Show resolved Hide resolved
packages/components/src/composite/stories/index.story.tsx Outdated Show resolved Hide resolved
packages/components/src/composite/index.ts Outdated Show resolved Hide resolved
packages/components/src/composite/index.ts Outdated Show resolved Hide resolved
packages/components/src/composite/index.ts Outdated Show resolved Hide resolved
packages/components/src/composite/index.ts Outdated Show resolved Hide resolved
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.

This LGTM, and tests well 👍

I acknowledge that there are a bunch of follow-ups after merging this one, and that's more than fine. Smaller changes always win!

🚀

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.

Waiting for an agreement on #63714, but just to add a concrete example to what I'm suggesting in #63714 (comment), I think Composite is a case where we don't need a .Root. The basic, default API should always start with MyComponent, and only when we need to provide a more low-level API in parallel, do we need a MyComponent.Root. Most consumers should never have to deal with a .Root. It's almost like the escape hatch for us as a component library. What do you think?

@tyxla
Copy link
Member

tyxla commented Jul 20, 2024

Waiting for an agreement on #63714, but just to add a concrete example to what I'm suggesting in #63714 (comment), I think Composite is a case where we don't need a .Root. The basic, default API should always start with MyComponent, and only when we need to provide a more low-level API in parallel, do we need a MyComponent.Root. Most consumers should never have to deal with a .Root. It's almost like the escape hatch for us as a component library. What do you think?

Just pointing out that I agree with that and brought a similar point up previously in my review here.

@ciampo ciampo force-pushed the feat/stabilize-composite/promote branch from 760fbb8 to fde527a Compare July 31, 2024 16:42
@ciampo ciampo requested review from mirka and tyxla July 31, 2024 16:42
@ciampo
Copy link
Contributor Author

ciampo commented Jul 31, 2024

Hey @tyxla and @mirka , I updated the PR using the overloaded naming convention. Could you take another look?

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.

With a bunch of follow-ups to come as discussed earlier, this looks good to me! 🚀

Excited to see it stable!

@@ -15,7 +15,8 @@ import {
import { UseCompositeStatePlaceholder, transform } from './utils';

const meta: Meta< typeof UseCompositeStatePlaceholder > = {
title: 'Components/Composite',
// TODO: should we keep this story around? If so, how should we call it?
title: 'Components/Composite/Legacy',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep it like we keep other deprecated component stories (like Navigation)?

@ciampo ciampo force-pushed the feat/stabilize-composite/promote branch from e9e14e5 to c41ff65 Compare August 9, 2024 09:43
@ciampo
Copy link
Contributor Author

ciampo commented Aug 9, 2024

Thank you for the final round of review.

I've updated the README and marked the store prop on Composite as required as final tweaks.

I'll merge as soon as CI checks pass 🟢

@ciampo ciampo enabled auto-merge (squash) August 9, 2024 09:44
Copy link

github-actions bot commented Aug 9, 2024

Flaky tests detected in c41ff65.
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/10317162077
📝 Reported issues:

@ciampo ciampo merged commit 22e4cc2 into trunk Aug 9, 2024
61 checks passed
@ciampo ciampo deleted the feat/stabilize-composite/promote branch August 9, 2024 10:18
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 9, 2024
@ciampo ciampo added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Aug 12, 2024
getdave pushed a commit that referenced this pull request Aug 14, 2024
* Point legacy exports directly to the source (instead of folder root)

* Swap default folder export to new version

* Apply compound component naming

* Export new version from the package

* Update (fix) private APIs exports

* Update composite implementation to use new compound naming

* Update references to Composite inside components package

* Update Storybook entry points for both legacy and current

* Fix Storybook generated docs

* Add todo

* Remove unncecessary code

* CHANGELOG

* README

* Add JSDocs to Composite exports

* Move current implementation out of `current` folder

* Fix import in the legacy implementation

* Update docs manifest

* Fix type in Storybook example

* Add JSDocs for Storybook docs

* Apply Overloaded naming convention

* Update README

* Fix typo

* Update legacy storybook title/id, make sure JSDocs refer to unstable version

* Derive types instead of importing them directly from ariakit

* Add JSDoc snippet for stable component

* Remove unnecessary JSDoc code

* Remove unnecessary display name

* Assign display names via Object.assign to comply with TS and get correct results in Storybook

* Update subcomponent TS ignore comment to align with other components

* Remove unnecessary store prop in circular option picker

Composite.Item should pick up the store from context without explicit prop

* Add first-party types, rewrite components with one unique forwardRef call

* Use the newly added types instead of using the Parameters<> util

* Fix Storybook story type

* Remove unnecessary ts-expect-error

* Use `CompositeStore` type directly

* Manual Storybook args table

* Tweak display name fallback

* README

* Mark `store` prop on `Composite` as required

---

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
@fabiankaegy fabiankaegy mentioned this pull request Oct 1, 2024
97 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Dev Note Requires a developer note for a major WordPress release cycle [Package] Components /packages/components [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants