DataViews: Type the ViewTable component #61682
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
commented
May 15, 2024
| @@ -28,7 +28,15 @@ interface FilterByConfig { | |||
| isPrimary?: boolean; | |||
| } | |||
|
|
|||
| type Operator = 'is' | 'isNot' | 'isAny' | 'isNone' | 'isAll' | 'isNotAll'; | |||
| type DeprecatedOperator = 'in' | 'notIn'; | |||
There was a problem hiding this comment.
@ntsekouras @oandregal are these still operators we support. (We do have code that references them, so I had to add them to the types)
There was a problem hiding this comment.
The only place we use them is this one https://github.com/WordPress/gutenberg/blob/trunk/packages/dataviews/src/utils.js#L20 for backward compatibility.
I think we should consider removing them at some point before 6.6, though they were part of 6.5 (in, not in). I presume we can still remove them as dataviews is a bundle package/experimental api?
There was a problem hiding this comment.
Yes, I think we can. Let's do that in a follow-up.
youknowriad
commented
May 15, 2024
| @@ -92,9 +147,9 @@ const HeaderMenu = forwardRef( function HeaderMenu( | |||
| variant="tertiary" | |||
| > | |||
| { field.header } | |||
| { isSorted && ( | |||
| { view.sort && isSorted && ( | |||
There was a problem hiding this comment.
Here, even if isSorted is actually enough to ensure view.sort is defined I had to add the extra check to make typescript happy. I wonder if there are other native typescript approaches to do that.
There was a problem hiding this comment.
Ok, this is an interesting one 🙂
I think the main problem is that the sort property is mutable, so TypeScript is very cautious about inferring its type based on code flow because the value could change. I've found two ways to fix this.
First - pull sort out into a constant, so when we check it and use it later we should know its value hasn't changed:
diff --git before/packages/dataviews/src/view-table.tsx after/packages/dataviews/src/view-table.tsx
index e39cd133414..247784b3118 100644
--- before/packages/dataviews/src/view-table.tsx
+++ after/packages/dataviews/src/view-table.tsx
@@ -123,9 +123,11 @@ const _HeaderMenu = forwardRef( function HeaderMenu< Item extends AnyItem >(
}: HeaderMenuProps< Item >,
ref: Ref< HTMLButtonElement >
) {
+ const sort = view.sort;
+
const isHidable = field.enableHiding !== false;
const isSortable = field.enableSorting !== false;
- const isSorted = view.sort?.field === field.id;
+ const isSorted = sort?.field === field.id;
const operators = sanitizeOperators( field );
// Filter can be added:
// 1. If the field is not already part of a view's filters.
@@ -150,9 +152,9 @@ const _HeaderMenu = forwardRef( function HeaderMenu< Item extends AnyItem >(
variant="tertiary"
>
{ field.header }
- { view.sort && isSorted && (
+ { isSorted && (
<span aria-hidden="true">
- { sortArrows[ view.sort.direction ] }
+ { sortArrows[ sort.direction ] }
</span>
) }
</Button>Alternatively, make view.sort property readonly:
diff --git before/packages/dataviews/src/types.ts after/packages/dataviews/src/types.ts
index 05ff4949204..2acb22798d7 100644
--- before/packages/dataviews/src/types.ts
+++ after/packages/dataviews/src/types.ts
@@ -155,7 +155,7 @@ interface ViewBase {
/**
* The sorting configuration.
*/
- sort?: {
+ readonly sort?: {
/**
* The field to sort by.
*/
diff --git before/packages/dataviews/src/view-table.tsx after/packages/dataviews/src/view-table.tsx
index e39cd133414..cf581a08140 100644
--- before/packages/dataviews/src/view-table.tsx
+++ after/packages/dataviews/src/view-table.tsx
@@ -150,7 +150,7 @@ const _HeaderMenu = forwardRef( function HeaderMenu< Item extends AnyItem >(
variant="tertiary"
>
{ field.header }
- { view.sort && isSorted && (
+ { isSorted && (
<span aria-hidden="true">
{ sortArrows[ view.sort.direction ] }
</span>Leveraging readonly tends to help TypeScript because it can make much stronger inferences knowing things are immutable.
There was a problem hiding this comment.
I see, I think we should do the first option for now, and maybe make everything readonly in a separate PR.
When TypeScript infers the type of an object literal like this:
{ key: 'value' }
It infers that the value is of type `string`. A const assertion tells
TypeScript it can treat the literal as a constant, so instead of
`string`, the type of the value can become the string literal `'value'`.
This fixes an issue where `string` would not satisfy a type that
expected a union of literal values. With the const assertion, TypeScript
knows that e.g. the type of the value `sortValues.asc` is `'ascending'`
instead of `string`.
|
Size Change: +121 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
This should be ready. |
ntsekouras
reviewed
May 16, 2024
| @@ -175,6 +183,22 @@ interface ViewBase { | |||
| hiddenFields: string[]; | |||
| } | |||
|
|
|||
| export interface ViewTable extends ViewBase { | |||
There was a problem hiding this comment.
We should also update type View to include this new one.
ntsekouras
reviewed
May 16, 2024
ntsekouras
reviewed
May 16, 2024
| data: Item[]; | ||
| fields: NormalizedField< Item >[]; | ||
| getItemId: ( item: Item ) => string; | ||
| isLoading?: boolean; |
There was a problem hiding this comment.
Should most of these props be part of the base ViewProps?
There was a problem hiding this comment.
You're right, we should definitely unify that type. I'll do that.
ntsekouras
approved these changes
May 16, 2024
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
May 27, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #55083
Follow-up to #61185
What?
The DataViews package is a types heavy package. There's a lot of structures that need to be documented properly. So this continues the effort to add explicit types to the package. This PR focuses on typing the ViewTable component.
Testing instructions
There's no functional change, it's essentially a code quality + documentation change.