Spinner: Convert component to TypeScript #41540

opr · 2022-06-03T14:28:15Z

What?

Converts the Spinner component to TypeScript

Why?

Brings us closer to completely typing the components package!

How?

I converted the relevant files to TS. I changed the story so there is now a default one, and one to allow editing of the height and width in a Custom Size story.

Testing Instructions

Run npm run dev and ensure no typing errors.
Run npm run storybook:dev and visit the Spinner component.
Ensure you can change the height/width props in the Custom Size story and that the Spinner displays correctly at different sizes without the stroke width changing.

Screenshots or screencast

cc @ciampo

ciampo

Hey @opr , thank you for your effort — it's really appreciated!

Could you also add an entry to the CHANGELOG ? Thank you!

ciampo · 2022-06-05T17:34:38Z

packages/components/src/spinner/index.tsx

If we use the WordPressComponentProps utility type instead, we can avoid specifying the className prop (and therefore, adding the types.ts file altogether):

diff --git a/packages/components/src/spinner/index.tsx b/packages/components/src/spinner/index.tsx index f1493a6c1c..ebe47628a2 100644 --- a/packages/components/src/spinner/index.tsx +++ b/packages/components/src/spinner/index.tsx @@ -7,12 +7,12 @@ import classNames from 'classnames'; * Internal dependencies */ import { StyledSpinner, SpinnerTrack, SpinnerIndicator } from './styles'; -import type { SpinnerProps } from './types'; +import type { WordPressComponentProps } from '../ui/context'; export default function Spinner( { className, ...props -}: SpinnerProps ): JSX.Element { +}: WordPressComponentProps< {}, 'svg', false > ) { return ( <StyledSpinner className={ classNames( 'components-spinner', className ) } diff --git a/packages/components/src/spinner/types.ts b/packages/components/src/spinner/types.ts deleted file mode 100644 index 596dafdb7a..0000000000 --- a/packages/components/src/spinner/types.ts +++ /dev/null @@ -1,3 +0,0 @@ -export interface SpinnerProps extends React.ComponentPropsWithoutRef< 'svg' > { - className: string; -}

The WordPressComponentProps< {}, 'svg', false > ) expression means:

use the WordPressComponentProps type

don't add any extra props (hence the {})

add all props and attributes of the svg element (including className)

mark the component as non-polymorphic by not accepting the as prop (hence the false), since it should always be rendered as an SVG

We also don't need to explicitly add JSX.Element as a return value.

Thanks for taking the time to explain this @ciampo this information is really valuable. I'll take this into account on future PRs.

We also don't need to explicitly add JSX.Element as a return value.

Good to know, thank you :)

ciampo · 2022-06-05T17:55:14Z

packages/components/src/spinner/stories/index.tsx

Let's make a few changes to how this story is written:

we need to add a snippet of code to the meta objects to setup controls and show the docs tab by default

while we're at it, we could actually remove the extra size control (as it's not a prop on the Spinner component), and instead just create a separate story that illustrates how the Spinner component can be resized to any size, while the stroke width stays unaffected

Sample diff of the changes

diff --git a/packages/components/src/spinner/stories/index.tsx b/packages/components/src/spinner/stories/index.tsx index e4f8d9c026..2ffbf680d8 100644 --- a/packages/components/src/spinner/stories/index.tsx +++ b/packages/components/src/spinner/stories/index.tsx @@ -1,51 +1,34 @@ /** * External dependencies */ -import { css } from '@emotion/react'; import type { Meta, Story } from '@storybook/react'; /** * Internal dependencies */ -import { useCx } from '../../utils'; import { space } from '../../ui/utils/space'; import Spinner from '../'; -const sizes = { - default: undefined, - medium: space( 20 ), - large: space( 40 ), -}; - -const meta: Meta< - typeof Spinner & { size: 'default' | 'medium' | 'large' } -> = { - title: 'Components/Spinner', +const meta: Meta< typeof Spinner > = { component: Spinner, - argTypes: { - size: { - options: Object.keys( sizes ), - mapping: sizes, - control: { type: 'select' }, - }, + title: 'Components/Spinner', + parameters: { + controls: { expanded: true }, + docs: { source: { state: 'open' } }, }, }; export default meta; -const Template: Story< - typeof Spinner & { size: 'default' | 'medium' | 'large' } -> = ( { size } ) => { - const cx = useCx(); - const spinnerClassname = cx( css` - width: ${ size }; - height: ${ size }; - ` ); - return <Spinner className={ spinnerClassname } />; -}; +const Template: Story< typeof Spinner > = ( args ) => <Spinner { ...args } />; + +export const Default: Story< typeof Spinner > = Template.bind( {} ); +Default.args = {}; -export const Default: Story< - typeof Spinner & { size: 'default' | 'medium' | 'large' } -> = Template.bind( {} ); -Default.args = { - size: 'default', +// The width of the Spinner's border is not affected by the overall component's dimensions. +export const CustomSize: Story< typeof Spinner > = Template.bind( {} ); +CustomSize.args = { + style: { + width: space( 20 ), + height: space( 20 ), + }, };

This seems much better thanks for the tip! Btw I used ComponentMeta and ComponentStory as they seem more correct since we're documenting React components, other components' stories use these types too.

packages/components/src/spinner/stories/index.tsx

ciampo

Apart from a small comment above, code changes LGTM!

Just wanted to double-check with @mirka about the props inherited from WordPressComponentProps not showing up in Storybook — is that expected?

chad1008

I'm still new to Typescript so I'll defer to @ciampo's approval there, but this looks good to me from a Storybook perspective! I'm also curious about the WordPressComponentProps not loading in the docs like @ciampo pointed out, but I think we can explore that further in another PR.

mirka · 2022-06-09T19:44:13Z

Just wanted to double-check with @mirka about the props inherited from WordPressComponentProps not showing up in Storybook — is that expected?

It's expected that the inherited props (i.e. second arg of WordPressComponentProps<>) is not shown, as that would be too much. However, the code in this PR also prevents Spinner's own props from showing — which is currently unnoticeable because Spinner doesn't have its own props (yet) 😄

To get the docgen to work, we need a named export:

// This named export is necessary for the docgen to work
export function Spinner() { /* ... */ }

export default Spinner;

Then, doing that shows me this in the props table:

Apparently this is suppressed by taking a forwardedRef arg like this. We might as well forward this ref, I guess?

ciampo · 2022-06-10T05:46:37Z

Good catch, @mirka !

To get the docgen to work, we need a named export:

I wasn't expecting any props to show in the table and therefore I didn't look for the named export 😅

Apparently this is suppressed by taking a forwardedRef arg like this. We might as well forward this ref, I guess?

Yes, agreed. Let's wrap the component in a forwardRef call

mirka · 2022-06-10T14:58:05Z

Sorry, changed my mind. Given that adding ref forwarding could be a breaking change, and we don't have a solid use case for forwarding the ref, I think it may be better to omit the ref for now. (As was done for RadioControl.) At the very least, it's a runtime change that shouldn't be part of this TS migration PR. What do you think?

ciampo · 2022-06-10T17:38:31Z

What do you think?

I'm not sure about how this can introduce a breaking change in the case of the Spinner component — as of today, the component wasn't using ref from its own props, and I don't think that we fall in the scenarios highlighted here.

I think we may be able to introduce forwardRef safely

mirka · 2022-06-10T20:38:39Z

Ok, yeah... I think I'm convinced that it's safe, and we might as well do it here instead of omitting.

opr · 2022-06-10T21:52:41Z

@ciampo and @mirka - I added fda74b1 - I copied other components that were using contextConnect as this seems the way the other do it, is this what you had in mind?

I also note that the ref prop is missing from Storybook now :)

Let me know if I should have done it differently or if this is fine.

Thanks! 😁

ciampo · 2022-06-13T12:16:30Z

Thank you for the update, @opr !

I had a look at your latest changes:

a requirement of the migration is that we add, next to the default export, a named export of the component for the version that is wrapped in the ref forwarding. For that component, we need to use the
in your current approach, your exporting the non-forwarded component (Spinner) and you're calling the connected component ConnectedSpinner. Instead we should:
- Name the "raw" component UnconnectedSpinner
- Name the connected component Spinner
- Name-export the connected component (Spinner), instead of the "raw" component
Finally, we have two choices when it comes to adding ref forwarding
- Use React's forwardRef
- Use the utility function contextConnect, which hooks the component into the library's context system
- If we wanted to use the context system, we should also call the useContextSystem hook in the component's code (as explained in the contributing guidelines highlighted in the previous point)
- Since the component doesn't currently use the context system, my preference would be to use the simpler forwardRef function from React

Here's how I would apply the feedback to this PR

diff --git a/packages/components/src/spinner/index.tsx b/packages/components/src/spinner/index.tsx
index c36778161f..60a1d787cd 100644
--- a/packages/components/src/spinner/index.tsx
+++ b/packages/components/src/spinner/index.tsx
@@ -1,17 +1,23 @@
 /**
  * External dependencies
  */
+import type { ForwardedRef } from 'react';
 import classNames from 'classnames';
 
+/**
+ * WordPress dependencies
+ */
+import { forwardRef } from '@wordpress/element';
+
 /**
  * Internal dependencies
  */
 import { StyledSpinner, SpinnerTrack, SpinnerIndicator } from './styles';
-import { WordPressComponentProps, contextConnect } from '../ui/context';
+import type { WordPressComponentProps } from '../ui/context';
 
-export function Spinner(
+function UnforwardedSpinner(
 	{ className, ...props }: WordPressComponentProps< {}, 'svg', false >,
-	forwardedRef: React.ForwardedRef< any >
+	forwardedRef: ForwardedRef< any >
 ) {
 	return (
 		<StyledSpinner
@@ -40,6 +46,18 @@ export function Spinner(
 	);
 }
 
-const ConnectedSpinner = contextConnect( Spinner, 'Spinner' );
+/**
+ * `Spinner` is a component used to notify users that their action is being processed.
+ *
+ * @example
+ * ```js
+ *   import { Spinner } from '@wordpress/components';
+ *
+ *   function Example() {
+ *     return <Spinner />;
+ *   }
+ * ```
+ */
+export const Spinner = forwardRef( UnforwardedSpinner );
 
-export default ConnectedSpinner;
+export default Spinner;

Finally, I'm sorry if this aspect of the TypeScript migration was not super clear, but we really appreciate your help on highlighting any friction and allow us to improve the process — in fact we're working on enhancing the guidelines in #41669!

opr · 2022-06-16T08:34:51Z

@ciampo thanks for your feedback! I have applied the changes you suggested. Your comment was super helpful.

It is no longer needed since we'll be using WordPressComponentProps

Co-authored-by: Marco Ciampini <[email protected]>

ciampo · 2022-06-17T12:31:56Z

Thank you again, @opr !

Hope to collaborate with you more often in the future :)

opr requested a review from ajitbohra as a code owner June 3, 2022 14:28

ciampo requested review from chad1008, ciampo and mirka June 3, 2022 18:10

ciampo assigned opr Jun 3, 2022

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components [Feature] Component System WordPress component system labels Jun 3, 2022

ciampo reviewed Jun 5, 2022

View reviewed changes

opr requested a review from ciampo June 6, 2022 13:12

ciampo reviewed Jun 9, 2022

View reviewed changes

packages/components/src/spinner/stories/index.tsx Outdated Show resolved Hide resolved

ciampo approved these changes Jun 9, 2022

View reviewed changes

chad1008 approved these changes Jun 9, 2022

View reviewed changes

ciampo mentioned this pull request Jun 9, 2022

Refactor @wordpress/components package to TypeScript #35744

Closed

opr requested a review from ciampo June 10, 2022 21:53

opr added 7 commits June 16, 2022 12:07

Add type definitions for Spinner component

9d9dbca

Rename spinner styles extension to .ts

64bfefa

Remove JSDoc style types from Spinner and replace with imported TS types

d74a403

Update Spinner storybook file to use TS

cf6601c

Add Spinner conversion to changelog

cbfc00e

Remove spinner types.ts file

6f93253

It is no longer needed since we'll be using WordPressComponentProps

Update Spinner to use WordPressComponentProps type

340e575

opr and others added 4 commits June 16, 2022 12:09

Update Spinner story to allow size changing through controls

a0b34fd

Update packages/components/src/spinner/stories/index.tsx

6d9e713

Co-authored-by: Marco Ciampini <[email protected]>

Forward ref in Spinner

4ee97a0

Export forwarded component, add example & use forwardRef from wp/element

df3885f

opr force-pushed the update/spinner-typescript branch from 76a4502 to df3885f Compare June 16, 2022 11:09

opr and others added 2 commits June 16, 2022 12:49

Update post-publish-panel snapshot to use Spinner with forwarded ref

7f9fcd3

Merge branch 'trunk' into update/spinner-typescript

7aa8e14

ciampo merged commit c965181 into WordPress:trunk Jun 17, 2022

github-actions bot added this to the Gutenberg 13.6 milestone Jun 17, 2022

Spinner: Convert component to TypeScript #41540

Spinner: Convert component to TypeScript #41540

Uh oh!

Conversation

opr commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

ciampo Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

opr Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

ciampo Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opr Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ciampo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chad1008 left a comment

Choose a reason for hiding this comment

Uh oh!

mirka commented Jun 9, 2022

Uh oh!

ciampo commented Jun 10, 2022

Uh oh!

mirka commented Jun 10, 2022

Uh oh!

ciampo commented Jun 10, 2022

Uh oh!

mirka commented Jun 10, 2022

Uh oh!

opr commented Jun 10, 2022

Uh oh!

ciampo commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opr commented Jun 16, 2022

Uh oh!

ciampo commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

opr commented Jun 3, 2022 •

edited

Loading

ciampo Jun 5, 2022 •

edited

Loading

opr Jun 6, 2022 •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo commented Jun 13, 2022 •

edited

Loading