Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Storybook and TS migration of ErrorPlaceholder component #5294

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Storybook and TS migration of ErrorPlaceholder component #5294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6490f99
Convert `ErrorPlaceholder` and `ErrorMessage` to TypeScript
sunyatasattva Dec 1, 2021
dba384e
Add stories for `ErrorPlaceholder` and `ErrorMessage` (#5249)
sunyatasattva Dec 1, 2021
3283f37
Update references to `ErrorMessage` component to leave the file exten…
sunyatasattva Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 14 additions & 19 deletions ...onents/error-placeholder/error-message.js → ...nents/error-placeholder/error-message.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,21 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import PropTypes from 'prop-types';
import { escapeHTML } from '@wordpress/escape-html';

const getErrorMessage = ( { message, type } ) => {
/**
* Internal dependencies
*/
import { ErrorObject } from '.';

export interface ErrorMessageProps {
/**
* The error object.
*/
error: ErrorObject;
}

const getErrorMessage = ( { message, type }: ErrorObject ) => {
if ( ! message ) {
return __(
'An unknown error occurred which prevented the block from being updated.',
Expand Down Expand Up @@ -42,24 +53,8 @@ const getErrorMessage = ( { message, type } ) => {
return message;
};

const ErrorMessage = ( { error } ) => (
const ErrorMessage = ( { error }: ErrorMessageProps ): JSX.Element => (
<div className="wc-block-error-message">{ getErrorMessage( error ) }</div>
);

ErrorMessage.propTypes = {
/**
* The error object.
*/
error: PropTypes.shape( {
/**
* Human-readable error message to display.
*/
message: PropTypes.node,
/**
* Context in which the error was triggered. That will determine how the error is displayed to the user.
*/
type: PropTypes.oneOf( [ 'api', 'general' ] ),
} ),
};

export default ErrorMessage;
70 changes: 38 additions & 32 deletions ...tor-components/error-placeholder/index.js → ...or-components/error-placeholder/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,53 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import PropTypes from 'prop-types';
import { Icon, notice } from '@woocommerce/icons';
import classNames from 'classnames';
import { Button, Placeholder, Spinner } from '@wordpress/components';

/**
* Internal dependencies
*/
import ErrorMessage from './error-message.js';
import ErrorMessage from './error-message';
import './editor.scss';

const ErrorPlaceholder = ( { className, error, isLoading, onRetry } ) => (
export interface ErrorObject {
/**
* Human-readable error message to display.
*/
message: string;
/**
* Context in which the error was triggered. That will determine how the error is displayed to the user.
*/
type: 'api' | 'general';
Copy link
Contributor

Choose a reason for hiding this comment

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

In these cases, I prefer to use an enum. This will allow for easy refactor in the future if there should be a need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I am also a fan of enums, I feel there are different use-cases which fits best each type, and one shouldn't default to enums, in general. Here is a quick rundown: https://blog.bam.tech/developer-news/should-you-use-enums-or-union-types-in-typescript and here also a nice SO opinion: https://stackoverflow.com/a/60041791/313115

In general, for example, enums are great for readability when their key is not equivalent to their value. Imagine something like:

{
  keycode: 37 | 38 | 39 | 40
}

This doesn't give the reader much information on what's going on. Rather:

enum Keycode {
  ARROW_LEFT = 37,
  ARROW_UP = 38,
  ARROW_RIGHT = 39,
  ARROW_DOWN = 40
}

// ...

{
  keycode: Keycode
}

The above is much more terse. In our case, the enum would be something like:

enum ErrorType {
  API = 'api'
  GENERAL = 'general'
}

I wouldn't say that's very helpful on the readability. I understand what you are saying regarding refactoring, but the SO answer above mentions iterating through a const, which I think it fits our case better. Something like:

const errorTypes = [ 'api', 'general' ] as const;
type ErrorTypes = typeof errorTypes[number];

Anyways, in this case, as we have only two types, I think this is slight premature optimization, so my opinion here is YAGNI.

But that's just my 2c, curious to hear what do you think. 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sending me these resources!
About readability you can define an enum like this:

enum Error = {
API,
GENERAL
}

For me, the advantage of enums (and the killer feature too) is that we can easily rename for the entire project, instead of the string literal.

Anyways, in this case, as we have only two types, I think this is slight premature optimization, so my opinion here is YAGNI.

I agree, but potentially this component it's a "global" component, so it is better to think about it now that refactor later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, the advantage of enums (and the killer feature too) is that we can easily rename for the entire project, instead of the string literal.

Mm, I see what you mean! However you could do the same renaming globally by just extracting the union type. There is no advantage in terms of refactoring between doing:

enum ErrorType = {
  API,
  GENERAL
}

and

type ErrorType = 'api' | 'general';

As long as you export both of those things and import them, refactoring is going to involve the exact amount of work. So I'm not sure if that's a significant advantage.

With that said, obviously, in this specific case it is actually a “hard-coded” string union type, so for refactoring reasons, definitely it might be better to extract it into its own type.

As a matter of fact, if you see the code changes, that's an approach I took elsewhere. Previously the code had:

error: PropTypes.shape( {
		/**
		 * Human-readable error message to display.
		 */
		message: PropTypes.node,
		/**
		 * Context in which the error was triggered. That will determine how the error is displayed to the user.
		 */
		type: PropTypes.oneOf( [ 'api', 'general' ] ),
	} )

The above was already repeated twice, so I refactored it in its own type. Some go by the Rule of Three when refactoring, though I am a bit more extreme and I usually refactor earlier (some say to a fault). But I definitely agree with you there about long term thinking and also, again, I am a fan of using Enums! 🤓

}

export interface ErrorPlaceholderProps {
/**
* Classname to add to placeholder in addition to the defaults.
*/
className?: string;
/**
* The error object.
*/
error: ErrorObject;
/**
* Whether there is a request running, so the 'Retry' button is hidden and
* a spinner is shown instead.
*/
isLoading: boolean;
/**
* Callback to retry an action.
*/
onRetry?: () => void;
}

const ErrorPlaceholder = ( {
className,
error,
isLoading = false,
onRetry,
}: ErrorPlaceholderProps ): JSX.Element => (
<Placeholder
icon={ <Icon srcElement={ notice } /> }
label={ __(
Expand All @@ -37,33 +72,4 @@ const ErrorPlaceholder = ( { className, error, isLoading, onRetry } ) => (
</Placeholder>
);

ErrorPlaceholder.propTypes = {
/**
* Classname to add to placeholder in addition to the defaults.
*/
className: PropTypes.string,
/**
* The error object.
*/
error: PropTypes.shape( {
/**
* Human-readable error message to display.
*/
message: PropTypes.node,
/**
* Context in which the error was triggered. That will determine how the error is displayed to the user.
*/
type: PropTypes.oneOf( [ 'api', 'general' ] ),
} ),
/**
* Whether there is a request running, so the 'Retry' button is hidden and
* a spinner is shown instead.
*/
isLoading: PropTypes.bool,
/**
* Callback to retry an action.
*/
onRetry: PropTypes.func,
};

export default ErrorPlaceholder;
27 changes: 27 additions & 0 deletions assets/js/editor-components/error-placeholder/stories/error-message.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* External dependencies
*/
import { Story, Meta } from '@storybook/react';

/**
* Internal dependencies
*/
import ErrorMessage, { ErrorMessageProps } from '../error-message';

export default {
title: 'WooCommerce Blocks/editor-components/Errors/Base Error Atom',
component: ErrorMessage,
} as Meta< ErrorMessageProps >;

const Template: Story< ErrorMessageProps > = ( args ) => (
<ErrorMessage { ...args } />
);

export const BaseErrorAtom = Template.bind( {} );
BaseErrorAtom.args = {
error: {
message:
'A very generic and unhelpful error. Please try again later. Or contact support. Or not.',
type: 'general',
},
};
70 changes: 70 additions & 0 deletions assets/js/editor-components/error-placeholder/stories/error-placeholder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* External dependencies
*/
import { Story, Meta } from '@storybook/react';
import { useArgs } from '@storybook/client-api';

/**
* Internal dependencies
*/
import ErrorPlaceholder, { ErrorPlaceholderProps } from '..';

export default {
title: 'WooCommerce Blocks/editor-components/Errors',
component: ErrorPlaceholder,
} as Meta< ErrorPlaceholderProps >;

const Template: Story< ErrorPlaceholderProps > = ( args ) => {
const [ { isLoading }, setArgs ] = useArgs();

const onRetry = args.onRetry
? () => {
setArgs( { isLoading: true } );

setTimeout( () => setArgs( { isLoading: false } ), 3500 );
}
: undefined;

return (
<ErrorPlaceholder
{ ...args }
onRetry={ onRetry }
isLoading={ isLoading }
/>
);
};

export const Default = Template.bind( {} );
Default.args = {
error: {
message:
'A very generic and unhelpful error. Please try again later. Or contact support. Or not.',
type: 'general',
},
};

export const APIError = Template.bind( {} );
APIError.args = {
error: {
message: 'Server refuses to comply. It is a teapot.',
type: 'api',
},
};

export const UnknownError = Template.bind( {} );
UnknownError.args = {
error: {
message: '',
type: 'general',
},
};

export const NoRetry: Story< ErrorPlaceholderProps > = ( args ) => {
return <ErrorPlaceholder { ...args } onRetry={ undefined } />;
};
NoRetry.args = {
error: {
message: '',
type: 'general',
},
};
19 changes: 0 additions & 19 deletions assets/js/editor-components/error-placeholder/stories/index.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion assets/js/editor-components/product-attribute-term-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { SearchListControl, SearchListItem } from '@woocommerce/components';
import { SelectControl } from '@wordpress/components';
import { withInstanceId } from '@wordpress/compose';
import { withAttributes } from '@woocommerce/block-hocs';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message.js';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message';
import classNames from 'classnames';
import ExpandableSearchListItem from '@woocommerce/editor-components/expandable-search-list-item/expandable-search-list-item.tsx';

Expand Down
2 changes: 1 addition & 1 deletion assets/js/editor-components/product-category-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import PropTypes from 'prop-types';
import { SearchListControl, SearchListItem } from '@woocommerce/components';
import { SelectControl } from '@wordpress/components';
import { withCategories } from '@woocommerce/block-hocs';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message.js';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message';
import classNames from 'classnames';

/**
Expand Down
2 changes: 1 addition & 1 deletion assets/js/editor-components/product-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
withSearchedProducts,
withTransformSingleSelectToMultipleSelect,
} from '@woocommerce/block-hocs';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message.js';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message';
import classNames from 'classnames';
import ExpandableSearchListItem from '@woocommerce/editor-components/expandable-search-list-item/expandable-search-list-item.tsx';

Expand Down
2 changes: 1 addition & 1 deletion assets/js/editor-components/products-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { __, _n, sprintf } from '@wordpress/i18n';
import { SearchListControl } from '@woocommerce/components';
import PropTypes from 'prop-types';
import { withSearchedProducts } from '@woocommerce/block-hocs';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message.js';
import ErrorMessage from '@woocommerce/editor-components/error-placeholder/error-message';

/**
* The products control exposes a custom selector for searching and selecting
Expand Down
Loading