-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DataViews: Enable types #61185
DataViews: Enable types #61185
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import type { ReactNode } from 'react'; | ||
|
||
type Item = Record< string, any >; | ||
|
||
interface Option { | ||
value: any; | ||
label: string; | ||
} | ||
|
||
interface filterByConfig { | ||
operators?: string[]; | ||
isPrimary?: boolean; | ||
} | ||
|
||
export interface Field { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried looking at all of our existing "fields" to define this. It might not be 100% accurate yet. We're also lacking the enumerations for available types and operators but it's a decent start. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not familiar with this package so this may be off base. There's an alternative to defining a single interface that all the instances "fit," which is to make this a union of very specific types. If there's a specific set of known fields, a discriminated union can be more expressive and provide much more precise and relevant to developers working with the code.
Something to consider. I also wrote a longer form post about the idea. |
||
/** | ||
* The unique identifier of the field. | ||
*/ | ||
id: string; | ||
|
||
/** | ||
* The label of the field. Defaults to the id. | ||
*/ | ||
header?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should rename this one "label". header is too "table" specific wording. |
||
|
||
/** | ||
* Callback used to retrieve the value of the field from the item. | ||
* Defaults to `item[ field.id ]`. | ||
*/ | ||
getValue?: ( { item }: { item: Item } ) => any; | ||
|
||
/** | ||
* Callback used to render the field. Defaults to `field.getValue`. | ||
*/ | ||
render?: ( { item }: { item: Item } ) => ReactNode; | ||
|
||
/** | ||
* The width of the field column. | ||
*/ | ||
width?: string | number; | ||
youknowriad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* The minimum width of the field column. | ||
*/ | ||
maxWidth?: string | number; | ||
|
||
/** | ||
* The maximum width of the field column. | ||
*/ | ||
minWidth?: string | number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have three different "width" properties. It felt too much for me. Do we really need all of these. |
||
|
||
/** | ||
* Whether the field is sortable. | ||
*/ | ||
enableSorting?: boolean; | ||
|
||
/** | ||
* Whether the field is searchable. | ||
*/ | ||
enableGlobalSearch?: boolean; | ||
|
||
/** | ||
* Whether the field is filterable. | ||
*/ | ||
enableHiding?: boolean; | ||
|
||
/** | ||
* The list of options to pick from when using the field as a filter. | ||
*/ | ||
elements?: Option[]; | ||
|
||
/** | ||
* Filter config for the field. | ||
*/ | ||
filterBy?: filterByConfig; | ||
youknowriad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to https://github.com/WordPress/gutenberg/blob/add/typescript-dataviews/packages/README.md?plain=1#L196 this looks fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Came here to suggest that we should add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No important impact aside that third-party devs using the package will now see some types appearing (autocompletion and all) when they use the package. Previously the build-types folder was empty, now it has the types defined in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Though I'm a bit confused. I wanted to see what we had previously, so I went to trunk and did There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right I have the same issue. I think the problem is that "dataviews" is not on the root tsconfig.json file in trunk which means when running |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"$schema": "https://json.schemastore.org/tsconfig.json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"extends": "../../tsconfig.base.json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"compilerOptions": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"rootDir": "src", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"declarationDir": "build-types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"references": [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../a11y" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../components" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../compose" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../element" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../i18n" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../icons" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../keycodes" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../primitives" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ "path": "../private-apis" } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"include": [ "src/**/*.ts", "src/**/*.tsx" ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like the preferred way of including src but not typechecking
Suggested change
I think there's an ongoing problem with react and ariakit types that I expect to be fixed by #60796. Sometimes I get errors like this:
We can work around that in this PR if its a problem by adding |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
Isn't the type missing here?
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.
no, in typescript, it's not needed because it's actually defined as part of the 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.
Oh, I hadn't noticed the file had been switched to TS. Do we need to switch our code to TypeScript for us to use the TS types in the docs? I thought we could use them in the JSDoc directly.
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.
@oandregal you can use them in JSDocs. I switched to typescript extension because I didn't want to list all the JS files that are fully type-checked in tsconfig file. the tsconfig file only targets all "ts" files for now.