-
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
Convert FocalPointPicker tests to TypeScript #61373
Convert FocalPointPicker tests to TypeScript #61373
Conversation
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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @jpstevens! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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 for working on it 👍
I think this looks good, I've only offered a couple of minor suggestions.
|
||
const props: FocalPointPickerMediaProps = { | ||
alt: '', | ||
src: '', |
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.
Literally every test will have the data-testid="media"
prop, should we add this to props
and remove that prop from the tests?
We might need to add the data-testid
prop to FocalPointPickerMediaProps
since that prop doesn't exist there.
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 question - I've updated the default prop object to include 'data-testid': 'media'
The change is only required for testing, so I opted not to change the underlying FocalPointPickerMediaProps
type definition.
The current implementation of Media
uses pass-through props, which allow any prop to be passed to the child component, including data-testid
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.
Interestingly, I couldn't find any tests in the codebase where the data-testid
prop is not repeated on each component:
- Elevation repeats
data-testid="elevation"
- Grid repeats
data-testid="grid"
- Panel repeats
data-testid="inner-content"
There is one example that extracts the data-testid
value to a const, but it doesn't prevent the prop repetition on the components:
- Icon repeats
data-testid={ testId }
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 for taking such a thorough look! This was a very minor detail and TBH I'm good with you've come up with 👍
Co-authored-by: Marin Atanasov <[email protected]>
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 good. Happy to ship this for you, but could I ask you to rebase it to the latest trunk
before that? Maybe that will solve the compressed size check failure.
|
||
const props: FocalPointPickerMediaProps = { | ||
alt: '', | ||
src: '', |
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 for taking such a thorough look! This was a very minor detail and TBH I'm good with you've come up with 👍
I synced with |
What?
This PR converts the
index
andmedia
tests for theFocalPointPicker
to TypeScript.Why?
Adding TypeScript support resolves this issue: #60529
How?
.tsx
{ ...props }
to components (similar to this test)"should handle legacy string values"
test, cast string valuesas unknown as number
to avoid changingFocalPoint
type to officially supportstring
values as well asnumber
If the preference is to officially support string values for focal points let me know and I'll update
FocalPoint
type toRecord< FocalPointAxis, number | string >
- however this will require further changes to theFocalPointPicker
implementation.Testing Instructions
npm run test:unit -- packages/components/src/focal-point-picker
tests pass (see screenshot below)npm run build:package-types
returns a zero exit codeScreenshots or screencast