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

Replace propTypes with TypeScript definitions #9679

Closed
wants to merge 13 commits into from
19 changes: 2 additions & 17 deletions assets/js/blocks/active-filters/block.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { useQueryStateByKey } from '@woocommerce/base-context/hooks';
import { getSetting, getSettingWithCoercion } from '@woocommerce/settings';
import { useMemo, useEffect, useState } from '@wordpress/element';
import classnames from 'classnames';
import PropTypes from 'prop-types';
import Label from '@woocommerce/base-components/label';
import {
isAttributeQueryCollection,
Expand Down Expand Up @@ -34,7 +33,7 @@ import {
} from './utils';
import ActiveAttributeFilters from './active-attribute-filters';
import FilterPlaceholders from './filter-placeholders';
import { Attributes } from './types';
import { ActiveFiltersBlockProps } from './types';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { ActiveFiltersBlockProps } from './types';
import type { ActiveFiltersBlockProps } from './types';

Using import type { ActiveFiltersBlockProps } from './types'; instead of import { ActiveFiltersBlockProps } from './types'; leads to a faster compile time. I'd say we should do that in this case, given that ActiveFiltersBlockProps is a type that we're importing.

Copy link
Author

Choose a reason for hiding this comment

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

For sure! thanks for the explanation this is valuable information.

import { useSetWraperVisibility } from '../filter-wrapper/context';

/**
Expand All @@ -47,10 +46,7 @@ import { useSetWraperVisibility } from '../filter-wrapper/context';
const ActiveFiltersBlock = ( {
attributes: blockAttributes,
isEditor = false,
}: {
attributes: Attributes;
isEditor?: boolean;
} ) => {
}: ActiveFiltersBlockProps ) => {
const setWrapperVisibility = useSetWraperVisibility();
const isMounted = useIsMounted();
const componentHasMounted = isMounted();
Expand Down Expand Up @@ -413,15 +409,4 @@ const ActiveFiltersBlock = ( {
);
};

ActiveFiltersBlock.propTypes = {
/**
* The attributes for this block.
*/
attributes: PropTypes.object.isRequired,
/**
* Whether it's in the editor or frontend display.
*/
isEditor: PropTypes.bool,
};

export default ActiveFiltersBlock;
11 changes: 11 additions & 0 deletions assets/js/blocks/active-filters/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,14 @@ export interface Attributes {
displayStyle: string;
className?: string;
}

export interface ActiveFiltersBlockProps {
/**
* The attributes for this block.
*/
attributes: Attributes;
/**
* Whether it's in the editor or frontend display.
*/
isEditor: boolean;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better if we change

export interface Attributes {
	heading: string;
	headingLevel: number;
	displayStyle: string;
	className?: string;
}

export interface ActiveFiltersBlockProps {
	/**
	 * The attributes for this block.
	 */
	attributes: Attributes;
	/**
	 * Whether it's in the editor or frontend display.
	 */
	isEditor: boolean;
}

to

export interface ActiveFiltersBlockProps {
	heading: string;
	headingLevel: number;
	displayStyle: string;
	className?: string;
	isEditor?: boolean;
}

That way, it's consistent with the other components. The definition isEditor?: boolean; means that this value is optional.