-
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
Guide
: refactor to TypeScript
#47493
Conversation
Size Change: +16 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
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.
Looking good! We also need a changelog before merge 🙏
pages = | ||
Children.map( children, ( child ) => ( { |
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.
I wish we had a readonly
for prop reassignment 😅
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.
Yes 😕
@@ -17,6 +17,7 @@ import { focus } from '@wordpress/dom'; | |||
import Modal from '../modal'; | |||
import Button from '../button'; | |||
import PageControl from './page-control'; | |||
import type { GuideProps } from './types'; | |||
|
|||
export default function Guide( { |
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 function needs to be named before the default export statement for the Storybook docgen to kick in.
function Guide( {
/* ... */
}
export default Guide;
This will make the type data show up in the props table in Storybook Docs view. And could we also add a main JSDoc to the component like the others?
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.
Flaky tests detected in 35c3d5a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4314356626
|
843cc06
to
62f7bf1
Compare
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.
Hey @flootr , would you also mind updating the component's README so that it's in sync with the JSDocs from types.ts
? In particular:
- all props should be listed
- we should update prop types to be more precise, using TypeScript types where possible (e.g from
function
to( event?: KeyboardEvent< HTMLDivElement > | SyntheticEvent ) => void
, fromarray
to{ content: ReactNode; image?: ReactNode; }[]
etc..) - we should double check which props are marked as required and which optional
- we should double check that the default value is listed too
Thank you! 🙏
62f7bf1
to
82d2080
Compare
@ciampo I think all feedback should be addressed. Let me know if there's anything else :) |
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 🚀
The Storybook example definitely needs some love, but that's a task for another day / PR (and not very high priority).
Feel free to merge once the last minor comments are addressed !
packages/components/CHANGELOG.md
Outdated
@@ -45,6 +45,7 @@ | |||
- `Navigator`: add more pattern matching tests, refine existing tests ([47910](https://github.com/WordPress/gutenberg/pull/47910)). | |||
- `ToolsPanel`: Refactor Storybook examples to TypeScript ([47944](https://github.com/WordPress/gutenberg/pull/47944)). | |||
- `ToolsPanel`: Refactor unit tests to TypeScript ([48275](https://github.com/WordPress/gutenberg/pull/48275)). | |||
- `Guide`: Convert to TypeScript ([#47493](https://github.com/WordPress/gutenberg/pull/47493)). |
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.
Looks like this needs to be moved again under the newer Unreleased
section of the CHANGELOG
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.
Yes. Fixed via 737ab84
82d2080
to
bbf3264
Compare
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
bbf3264
to
35c3d5a
Compare
What?
Refactor the
Guide
component to TypeScriptPart of #35744
Why?
The refactor to TypeScript has many benefits (auto-generated docs, static linting and error prevention, better IDE experience). See #35744 for more details
How?
Followed the steps highlighted in the TypeScript migration guide.
Testing Instructions