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. |
github-actions
bot
added
the
First-time Contributor
mirka
added
[Type] Automated Testing
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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 <8436925+tyxla@users.noreply.github.com>
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
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.
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 |
3 participants
What?
This PR converts the
indexandmediatests for theFocalPointPickerto 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 numberto avoid changingFocalPointtype to officially supportstringvalues as well asnumberIf the preference is to officially support string values for focal points let me know and I'll update
FocalPointtype toRecord< FocalPointAxis, number | string >- however this will require further changes to theFocalPointPickerimplementation.Testing Instructions
npm run test:unit -- packages/components/src/focal-point-pickertests pass (see screenshot below)npm run build:package-typesreturns a zero exit codeScreenshots or screencast