-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Type check docs examples #3710
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
Type check docs examples #3710
Conversation
|
Build successful! 🎉 |
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| ```tsx example render=false | ||
| ///- begin collapse -/// | ||
| function ListBox(props) { |
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 was unused. The real implementation is in the below example.
| <Tabs aria-label="History of Ancient Rome" items={tabs} onSelectionChange={setTabId}> | ||
| <TabList> | ||
| {item => ( | ||
| {(item: Tab) => ( |
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.
These are annoying. Because we designed the API with items on Tabs rather than TabList and TabPanels, this will always be unknown by default without an annotation.
|
|
||
| function Example() { | ||
| let [selected, setSelected] = React.useState(new Set(['cheese'])); | ||
| let [selected, setSelected] = React.useState<Selection>(new Set(['cheese'])); |
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.
These are annoying, and caused by onSelectionChange which may emit "all" | Set<Key>, but the type of this variable is only inferred to be Set<string> so it always requires an annotation.
…m into docs-ts # Conflicts: # packages/@adobe/react-spectrum/package.json # packages/@adobe/react-spectrum/src/index.ts # packages/@react-aria/dnd/src/index.ts # packages/@react-aria/overlays/docs/useOverlayTrigger.mdx # packages/@react-spectrum/autocomplete/src/SearchAutocomplete.tsx # packages/@react-spectrum/list/docs/ListView.mdx # packages/dev/parcel-transformer-mdx-docs/MDXTransformer.js
|
Build successful! 🎉 |
# Conflicts: # packages/@adobe/react-spectrum/package.json # packages/@react-spectrum/actionbar/docs/ActionBar.mdx
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
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.
LGTM, verified locally that running make check-examples was catching typescript issues in the docs. Spot checked a couple of the docs changes and verified that they rendered just fine in the docs.
snowystinger
left a comment
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.
lgtm, also spot checked the docs
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } |
Part of #865
This adds a script that extracts all of the examples from the docs MDX files into TSX files, and then runs TypeScript over them. This will run in CI. It found a number of issues, such as unused variables, missing props, etc. So far, I have only fixed the existing type errors, adding annotations where needed. Eventually, we can add type annotations to all examples.
We may want to eventually have a toggle to switch between TS and JS. For now, only TS is displayed.