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

ConfirmDialog: ts unit test storybook #54954

Merged

Conversation

margolisj
Copy link
Contributor

What?

Upgrades ConfirmDialog to TS.

Why?

Typescript is the only sane was to maintain a JS codebase this large.

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@margolisj
Copy link
Contributor Author

@mirka @ciampo #35744. Like always, lmk if there are any other changes necessary. Was working on this a week ago or so. Getting the following:
JSX element type 'ConfirmDialog' does not have any construct or call signatures.

I don't see what wrong exactly. Tried a few type combination.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

There seems to be an issue when using type unions for a component's prop type, and the contextConnect function.

For now, let's remove the type union so that we can complete the refactor
diff --git a/packages/components/src/confirm-dialog/component.tsx b/packages/components/src/confirm-dialog/component.tsx
index fa5ede6955..9c600e47bf 100644
--- a/packages/components/src/confirm-dialog/component.tsx
+++ b/packages/components/src/confirm-dialog/component.tsx
@@ -13,7 +13,7 @@ import { useCallback, useEffect, useRef, useState } from '@wordpress/element';
  * Internal dependencies
  */
 import Modal from '../modal';
-import type { OwnProps, DialogInputEvent } from './types';
+import type { ConfirmDialogProps, DialogInputEvent } from './types';
 import type { WordPressComponentProps } from '../context';
 import { useContextSystem, contextConnect } from '../context';
 import { Flex } from '../flex';
@@ -24,7 +24,7 @@ import * as styles from './styles';
 import { useCx } from '../utils/hooks/use-cx';
 
 const ConfirmDialog = (
-	props: WordPressComponentProps< OwnProps, 'div' >,
+	props: WordPressComponentProps< ConfirmDialogProps, 'div', false >,
 	forwardedRef: ForwardedRef< any >
 ) => {
 	const {
diff --git a/packages/components/src/confirm-dialog/types.ts b/packages/components/src/confirm-dialog/types.ts
index 72fef59dc2..1dd6bf77da 100644
--- a/packages/components/src/confirm-dialog/types.ts
+++ b/packages/components/src/confirm-dialog/types.ts
@@ -13,21 +13,11 @@ export type DialogInputEvent =
 	| KeyboardEvent< HTMLDivElement >
 	| MouseEvent< HTMLButtonElement >;
 
-type BaseProps = {
+export type ConfirmDialogProps = {
 	children: ReactNode;
 	onConfirm: ( event: DialogInputEvent ) => void;
 	confirmButtonText?: string;
 	cancelButtonText?: string;
-};
-
-type ControlledProps = BaseProps & {
-	onCancel: ( event: DialogInputEvent ) => void;
-	isOpen: boolean;
-};
-
-type UncontrolledProps = BaseProps & {
 	onCancel?: ( event: DialogInputEvent ) => void;
-	isOpen?: never;
+	isOpen?: boolean;
 };
-
-export type OwnProps = ControlledProps | UncontrolledProps;

On top of migrating the remaining files (like tests and storybook), could you also:

  • add the same info on each prop from the README as JSDocs on the types in types.ts ? (example)
  • add some info on the component as JSDoc (this can also be borrowed from the README) (example)

@margolisj margolisj force-pushed the ConfirmDialog-ts-unit-test-storybook branch from 0733e0c to b8d46b2 Compare October 2, 2023 15:20
Copy link
Contributor

Choose a reason for hiding this comment

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

The Storybook examples are still not displaying correctly all of the inferred type information — we need to:

  • update the meta object, adding the correct configuration for generating controls and removing the hardcoded ones
  • export the ConfirmDialog component as a named export in component.tsx and use the name export in Storybook (there is abug that prevents storybook from generating controls from default exports)

In the process, we can also:

  • tweak the args configuration, so that it follows the same way we've been configuring it in other stories
  • tweak the code so that it doesn't need to import types explicitly
Here is a diff with all those changes applied
diff --git a/packages/components/src/confirm-dialog/component.tsx b/packages/components/src/confirm-dialog/component.tsx
index ff46d77c05..750e7030de 100644
--- a/packages/components/src/confirm-dialog/component.tsx
+++ b/packages/components/src/confirm-dialog/component.tsx
@@ -184,8 +184,8 @@ const UnconnectedConfirmDialog = (
  * }
  * ```
  */
-const ConnectedConfirmDialog = contextConnect(
+export const ConfirmDialog = contextConnect(
 	UnconnectedConfirmDialog,
 	'ConfirmDialog'
 );
-export default ConnectedConfirmDialog;
+export default ConfirmDialog;
diff --git a/packages/components/src/confirm-dialog/stories/index.story.tsx b/packages/components/src/confirm-dialog/stories/index.story.tsx
index 3eb72b2829..85636c0ddc 100644
--- a/packages/components/src/confirm-dialog/stories/index.story.tsx
+++ b/packages/components/src/confirm-dialog/stories/index.story.tsx
@@ -12,32 +12,21 @@ import { useState } from '@wordpress/element';
  * Internal dependencies
  */
 import Button from '../../button';
-import { ConfirmDialog } from '..';
-import type { ConfirmDialogProps, DialogInputEvent } from '../types';
+import { ConfirmDialog } from '../component';
 
 const meta: Meta< typeof ConfirmDialog > = {
 	component: ConfirmDialog,
 	title: 'Components (Experimental)/ConfirmDialog',
 	argTypes: {
-		children: {
-			control: { type: 'text' },
-		},
-		confirmButtonText: {
-			control: { type: 'text' },
-		},
-		cancelButtonText: {
-			control: { type: 'text' },
-		},
 		isOpen: {
 			control: { type: null },
 		},
-		onConfirm: { action: 'onConfirm' },
-		onCancel: { action: 'onCancel' },
-	},
-	args: {
-		children: 'Would you like to privately publish the post now?',
 	},
 	parameters: {
+		actions: { argTypesRegex: '^on.*' },
+		controls: {
+			expanded: true,
+		},
 		docs: { canvas: { sourceState: 'shown' } },
 	},
 };
@@ -48,15 +37,15 @@ const Template: StoryFn< typeof ConfirmDialog > = ( {
 	onConfirm,
 	onCancel,
 	...args
-}: ConfirmDialogProps ) => {
+} ) => {
 	const [ isOpen, setIsOpen ] = useState( false );
 
-	const handleConfirm = ( confirmArgs: DialogInputEvent ) => {
+	const handleConfirm: typeof onConfirm = ( confirmArgs ) => {
 		onConfirm( confirmArgs );
 		setIsOpen( false );
 	};
 
-	const handleCancel = ( cancelArgs: DialogInputEvent ) => {
+	const handleCancel: typeof onCancel = ( cancelArgs ) => {
 		onCancel?.( cancelArgs );
 		setIsOpen( false );
 	};
@@ -80,7 +69,7 @@ const Template: StoryFn< typeof ConfirmDialog > = ( {
 };
 
 // Simplest usage: just declare the component with the required `onConfirm` prop. Note: the `onCancel` prop is optional here, unless you'd like to render the component in Controlled mode (see below)
-export const _default = Template.bind( {} );
+export const Default = Template.bind( {} );
 const _defaultSnippet = `() => {
   const [ isOpen, setIsOpen ] = useState( false );
   const [ confirmVal, setConfirmVal ] = useState('');
@@ -113,8 +102,10 @@ const _defaultSnippet = `() => {
     </>
   );
 };`;
-_default.args = {};
-_default.parameters = {
+Default.args = {
+	children: 'Would you like to privately publish the post now?',
+};
+Default.parameters = {
 	docs: {
 		source: {
 			code: _defaultSnippet,
@@ -127,6 +118,7 @@ _default.parameters = {
 // To customize button text, pass the `cancelButtonText` and/or `confirmButtonText` props.
 export const WithCustomButtonLabels = Template.bind( {} );
 WithCustomButtonLabels.args = {
+	...Default.args,
 	cancelButtonText: 'No thanks',
 	confirmButtonText: 'Yes please!',
 };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully I got that copied right. Really should just look up how to apply a git patch haha. Lmk if there's anything else.

Copy link
Contributor

Choose a reason for hiding this comment

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

After copying the contents of the patch, on MacOS I can do that by executing pbpaste | git apply

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully I got that copied right

Almost

@margolisj margolisj force-pushed the ConfirmDialog-ts-unit-test-storybook branch from 1f8849b to 7767d1e Compare October 5, 2023 15:17
@ciampo ciampo added the [Package] Components /packages/components label Oct 6, 2023
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ciampo ciampo enabled auto-merge (squash) October 6, 2023 13:00
@ciampo ciampo added the [Type] Enhancement A suggestion for improvement. label Oct 6, 2023
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Oct 6, 2023
@ciampo ciampo disabled auto-merge October 7, 2023 12:55
@ciampo
Copy link
Contributor

ciampo commented Oct 7, 2023

The failing e2e test doesn't seem related to the changes in this PR, will merge nonetheless.

@ciampo ciampo merged commit bd48f41 into WordPress:trunk Oct 7, 2023
49 of 51 checks passed
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Oct 7, 2023
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants