-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Refactor QueryControls
component to TypeScript
#46721
Conversation
It can also be an author, so Term isn't right.
- Required: No | ||
- Platform: Web | ||
|
||
#### selectedAuthorId | ||
#### `categoriesList`: `Entity[]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff looks strange because I alphabetized the props.
selectedAuthorId
is just moved lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll review the list of props in the README as one of the last items, after everything else looks good in code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time to update the README to follow the latest types specs, before final review :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I took at look at the README and made a few light tweaks:
- added clarity to the descriptions by stating clearly when we refer to props
- removed references to internal implementation details (e.g no mention of
SelectControl
andFormTokenField
) - when possible, "expanded" types in the README so that they are as straightforward as possible (I didn't expand
Author
andCategory
because I assumed that devs could look it up anyway).
Here is a diff with those changes
diff --git a/packages/components/src/query-controls/types.ts b/packages/components/src/query-controls/types.ts
index 6086d35e2a..a17f33cdf5 100644
--- a/packages/components/src/query-controls/types.ts
+++ b/packages/components/src/query-controls/types.ts
@@ -47,18 +47,17 @@ type OrderBy = 'date' | 'title';
type BaseQueryControlsProps = {
/**
- * An array of authors that is passed into
- * an `AuthorSelect` sub-component.
+ * An array of the authors to select from.
*/
authorList?: AuthorSelectProps[ 'authorList' ];
/**
- * The maximum items.
+ * The maximum number of items.
*
* @default 100
*/
maxItems?: number;
/**
- * The minimum of items.
+ * The minimum number of items.
*
* @default 1
*/
@@ -69,24 +68,24 @@ type BaseQueryControlsProps = {
numberOfItems?: number;
/**
* A function that receives the new author value.
- * If this is not specified, the author controls are not included.
+ * If this prop is not specified, the author controls are not included.
*/
onAuthorChange?: AuthorSelectProps[ 'onChange' ];
/**
* A function that receives the new number of items value.
- * If this is not specified, then the number of items
+ * If this prop is not specified, then the number of items
* range control is not included.
*/
onNumberOfItemsChange?: ( newNumber?: number ) => void;
/**
* A function that receives the new order value.
- * If this or onOrderByChange are not specified,
+ * If this prop or the `onOrderByChange` prop are not specified,
* then the order controls are not included.
*/
onOrderChange?: ( newOrder: Order ) => void;
/**
* A function that receives the new orderby value.
- * If this or onOrderChange are not specified,
+ * If this prop or the `onOrderChange` prop are not specified,
* then the order controls are not included.
*/
onOrderByChange?: ( newOrderBy: OrderBy ) => void;
@@ -107,17 +106,17 @@ type BaseQueryControlsProps = {
export type QueryControlsWithSingleCategorySelectionProps =
BaseQueryControlsProps & {
/**
- * An array of categories, renders a `CategorySelect`
- * sub-component when passed in conjunction with `onCategoryChange`.
+ * An array of categories, renders UI that allows selecting one category at
+ * a time when passed in conjunction with the `onCategoryChange` prop.
*/
categoriesList?: CategorySelectProps[ 'categoriesList' ];
/**
- * The selected category for the `categoriesList`.
+ * The selected category for the `categoriesList` prop.
*/
selectedCategoryId?: CategorySelectProps[ 'selectedCategoryId' ];
/**
* A function that receives the new category value.
- * If this is not specified, the category controls are not included.
+ * If this prop is not specified, the category controls are not included.
*/
onCategoryChange?: CategorySelectProps[ 'onChange' ];
};
@@ -125,14 +124,12 @@ export type QueryControlsWithSingleCategorySelectionProps =
export type QueryControlsWithMultipleCategorySelectionProps =
BaseQueryControlsProps & {
/**
- * An array of category names, renders a `FormTokenField` component
- * when passed in conjunction with `onCategoryChange`.
+ * An array of category names, renders UI that enables multiple selection
+ * when passed in conjunction with the `onCategoryChange` prop.
*/
- categorySuggestions?: {
- [ categoryName: Category[ 'name' ] ]: Category;
- };
+ categorySuggestions?: Record< Category[ 'name' ], Category >;
/**
- * The selected categories for the `categorySuggestions`.
+ * The selected categories for the `categorySuggestions` prop.
*/
selectedCategories?: Category[];
/**
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ciampo,
Alright, I took at look at the README and made a few light tweaks:
By README, did you mean types.ts
?
The diff above is only for types.ts
.
No problem, just wanted to double-check before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot — for some reason a good chunk of my proposed edits did not paste into my comment
Here's the remaining diff
diff --git a/packages/components/src/query-controls/README.md b/packages/components/src/query-controls/README.md
index 313244f12f..c34aae6f42 100644
--- a/packages/components/src/query-controls/README.md
+++ b/packages/components/src/query-controls/README.md
@@ -122,34 +122,34 @@ const MyQueryControls = () => {
};
```
-The format of the categories list also needs to be updated to match what `FormTokenField` expects for making suggestions.
+The format of the categories list also needs to be updated to match the expected type for the category suggestions.
### Props
-#### `authorList`: `AuthorSelectProps[ 'authorList' ]`
+#### `authorList`: `Author[]`
-An array of authors that is passed into an `AuthorSelect` sub-component.
+An array of the authors to select from.
- Required: No
- Platform: Web
-#### `categoriesList`: `CategorySelectProps[ 'categoriesList' ]`
+#### `categoriesList`: `Category[]`
-An array of categories, renders a `CategorySelect` sub-component when passed in conjunction with `onCategoryChange`.
+An array of categories. When passed in conjunction with the `onCategoryChange` prop, it causes the component to render UI that allows selecting one category at a time.
- Required: No
- Platform: Web
#### `categorySuggestions`: `Record< Category[ 'name' ], Category >`
-An object of categories, renders a `FormTokenField` component when passed in conjunction with `onCategoryChange`.
+An object of categories with the category name as the key. When passed in conjunction with the `onCategoryChange` prop, it causes the component to render UI that enables multiple selection.
- Required: No
- Platform: Web
#### `maxItems`: `number`
-The maximum of items.
+The maximum number of items.
- Required: No
- Default: 100
@@ -157,7 +157,7 @@ The maximum of items.
#### `minItems`: `number`
-The minimum of items.
+The minimum number of items.
- Required: No
- Default: 1
@@ -170,56 +170,56 @@ The selected number of items to retrieve via the query.
- Required: No
- Platform: Web
-#### `onAuthorChange`: `AuthorSelectProps[ 'onChange' ]`
+#### `onAuthorChange`: `( newAuthor: string ) => void`
-A function that receives the new author value. If this is not specified, the author controls are not included.
+A function that receives the new author value. If not specified, the author controls are not rendered.
- Required: No
- Platform: Web
#### `onCategoryChange`: `CategorySelectProps[ 'onChange' ] | FormTokenFieldProps[ 'onChange' ]`
-A function that receives the new category value. If this is not specified, the category controls are not included.
+A function that receives the new category value. If not specified, the category controls are not rendered.
- Required: No
- Platform: Web
#### `onNumberOfItemsChange`: `( newNumber?: number ) => void`
-A function that receives the new number of items value. If this is not specified, then the number of items range control is not included.
+A function that receives the new number of items. If not specified, then the number of items range control is not rendered.
- Required: No
- Platform: Web
-#### `onOrderChange`: `( newOrder: Order ) => void`
+#### `onOrderChange`: `( newOrder: 'asc' | 'desc' ) => void`
-A function that receives the new order value. If this or onOrderByChange are not specified, then the order controls are not included.
+A function that receives the new order value. If this prop or the `onOrderByChange` prop are not specified, then the order controls are not rendered.
- Required: No
- Platform: Web
-#### `onOrderByChange`: `( newOrderBy: OrderBy ) => void`
+#### `onOrderByChange`: `( newOrderBy: 'date' | 'title' ) => void`
-A function that receives the new orderby value. If this or onOrderChange are not specified, then the order controls are not included.
+A function that receives the new orderby value. If this prop or the `onOrderChange` prop are not specified, then the order controls are not rendered.
- Required: No
- Platform: Web
-#### `order`: `Order`
+#### `order`: `'asc' | 'desc'`
The order in which to retrieve posts.
- Required: No
- Platform: Web
-#### `orderBy`: `OrderBy`
+#### `orderBy`: `'date' | 'title'`
The meta key by which to order posts.
- Required: No
- Platform: Web
-#### `selectedAuthorId`: `AuthorSelectProps[ 'selectedAuthorId' ]`
+#### `selectedAuthorId`: `number`
The selected author ID.
@@ -228,14 +228,14 @@ The selected author ID.
#### `selectedCategories`: `Category[]`
-The selected categories for the `categorySuggestions`.
+The selected categories for the `categorySuggestions` prop.
- Required: No
- Platform: Web
-#### `selectedCategoryId`: `CategorySelectProps[ 'selectedCategoryId' ]`
+#### `selectedCategoryId`: `number`
-The selected category for the `categoriesList`.
+The selected category for the `categoriesList` prop.
- Required: No
- Platform: Web
diff --git a/packages/components/src/query-controls/types.ts b/packages/components/src/query-controls/types.ts
index a17f33cdf5..63bbfeaa5a 100644
--- a/packages/components/src/query-controls/types.ts
+++ b/packages/components/src/query-controls/types.ts
@@ -68,25 +68,25 @@ type BaseQueryControlsProps = {
numberOfItems?: number;
/**
* A function that receives the new author value.
- * If this prop is not specified, the author controls are not included.
+ * If not specified, the author controls are not rendered.
*/
onAuthorChange?: AuthorSelectProps[ 'onChange' ];
/**
- * A function that receives the new number of items value.
- * If this prop is not specified, then the number of items
- * range control is not included.
+ * A function that receives the new number of items.
+ * If not specified, then the number of items
+ * range control is not rendered.
*/
onNumberOfItemsChange?: ( newNumber?: number ) => void;
/**
* A function that receives the new order value.
* If this prop or the `onOrderByChange` prop are not specified,
- * then the order controls are not included.
+ * then the order controls are not rendered.
*/
onOrderChange?: ( newOrder: Order ) => void;
/**
* A function that receives the new orderby value.
* If this prop or the `onOrderChange` prop are not specified,
- * then the order controls are not included.
+ * then the order controls are not rendered.
*/
onOrderByChange?: ( newOrderBy: OrderBy ) => void;
/**
@@ -106,8 +106,9 @@ type BaseQueryControlsProps = {
export type QueryControlsWithSingleCategorySelectionProps =
BaseQueryControlsProps & {
/**
- * An array of categories, renders UI that allows selecting one category at
- * a time when passed in conjunction with the `onCategoryChange` prop.
+ * An array of categories. When passed in conjunction with the
+ * `onCategoryChange` prop, it causes the component to render UI that allows
+ * selecting one category at a time.
*/
categoriesList?: CategorySelectProps[ 'categoriesList' ];
/**
@@ -116,7 +117,7 @@ export type QueryControlsWithSingleCategorySelectionProps =
selectedCategoryId?: CategorySelectProps[ 'selectedCategoryId' ];
/**
* A function that receives the new category value.
- * If this prop is not specified, the category controls are not included.
+ * If not specified, the category controls are not rendered.
*/
onCategoryChange?: CategorySelectProps[ 'onChange' ];
};
@@ -124,8 +125,9 @@ export type QueryControlsWithSingleCategorySelectionProps =
export type QueryControlsWithMultipleCategorySelectionProps =
BaseQueryControlsProps & {
/**
- * An array of category names, renders UI that enables multiple selection
- * when passed in conjunction with the `onCategoryChange` prop.
+ * An object of categories with the category name as the key. When passed in
+ * conjunction with the `onCategoryChange` prop, it causes the component to
+ * render UI that enables multiple selection.
*/
categorySuggestions?: Record< Category[ 'name' ], Category >;
/**
@@ -134,7 +136,7 @@ export type QueryControlsWithMultipleCategorySelectionProps =
selectedCategories?: Category[];
/**
* A function that receives the new category value.
- * If this is not specified, the category controls are not included.
+ * If not specified, the category controls are not rendered.
*/
onCategoryChange?: FormTokenFieldProps[ 'onChange' ];
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll commit this today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed in 4aa1375
Thanks for your suggestions and code, Marco! Just finished applying your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience!
Next round of review should be the final one 🤞
- Required: No | ||
- Platform: Web | ||
|
||
#### selectedAuthorId | ||
#### `categoriesList`: `Entity[]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time to update the README to follow the latest types specs, before final review :)
Thanks a lot for your deep look at this, @ciampo! If it's alright, I'll apply your suggestions by tomorrow. |
Thanks, committed in 9e512a3 |
@ciampo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the big effort here!
Once the final changes to the README are addressed (including this comment), feel free to merge 🚀
Thanks, Marco! I'll do this later today, if that's alright. |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
There was only a conflict in components/CHANGELOG.md
Hi @ciampo, Thanks for your huge amount of work and research on this. |
I re-ran the tests and merged 🚀 Thank you for the patience and the collaboration, Ryan! |
Thanks a lot, Marco! |
What?
Convert the
QueryControls
component to TypeScriptWhy?
As part of an effort to convert
@wordpress/components
to TypeScriptHow?
Mainly by adding types to the
QueryControls
componentTesting Instructions
(That uses this QueryControls component)
Screenshots
See above